Page MenuHomePhabricator

Refactor mobile references code
Closed, DeclinedPublic

Description

Currently, this task captures the conversation around the refactoring of the [mobile.references](https://github.com/wikimedia/mediawiki-extensions-MobileFrontend/tree/master/resources/mobile.references) module.

 Remaining plan

  1. The ReferencesDrawer class is reduced to a simple view by extracting the logic from the #showNestedReference method, which tracks whether the reference drawer is already open, into the ReferencesController#onReferenceClick method.
  2. Add the ReferencesController class, which has one method, #onReferenceClick, which is invoked when the user clicks a reference, and maintains the contains the key state of the system: whether the references drawer is already open; whether a reference description is being fetched from the API (in the case of the lazily loaded references feature).

 Background

The conversation started during the code review of 278081 and is – roughly – as follows:

@phuedx:

[…] I'm not in favour of this addition to the ReferencesDrawer view class. Previously, the distinction between the controlling part of the references machinery (the code in the setup file) and the corresponding view was adequately separated. In this change, the ReferencesDrawer is now concerned with fetching references for reference links as well as rendering them. This function should actually be in the setup file.

One way of achieving this would be to trigger an event representing a reference click so that it can be handled elsewhere.

@Jdlrobson:

It feels like this should be inside the ReferenceDrawer so I think in a follow up we should explore moving a lot of this logic into the ReferencesDrawer itself.

Code I think we should move into ReferenceDrawer:

  • getReferenceData
  • getReference
  • mw.trackSubscribe

Thoughts?

@bmansurov:

We usually pass gateway, api and such to the View. So this looks more consistent to me.

@Jdlrobson:

Okay.. so if we have a ReferencesGateway, we'd pass it to ReferenceDrawer, this would mean this code could be done inside ReferenceDrawer using the ReferencesGateway.
Does that sound right?

@bmansurov:

Yes that sounds good.

Previously....

We split references.js out the CiteReferencesGateway and MobileViewReferencesGateway classes, which are responsible for fetching references data from some external service. All gateway class have a #getReferencesByRevID method; and the ReferencesGatewayFactory class, which constructs an instance of the appropriate gateway class based on some config, e.g. wgMFLazyLoadReferences.useMobileViewApi

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

(I'm trying to merge T130391 into this but doesn't work on my phone :/)

Added my notes from the "Review the design of mobile.references module" meeting held on Thursday, 6th April.

Change 275724 had a related patch set uploaded (by Jdlrobson):
Lazy load references via mobileview API

https://gerrit.wikimedia.org/r/275724

Okay had a go at this refactoring in the updated patch.

This is heavily tied with T129182 so I can only assume the unplanned sprint work tag was accidentally added.

This is heavily tied with T129182 so I can only assume the unplanned sprint work tag was accidentally added.

Though this was definitely Unplanned-Sprint-Work from Reading-Web-Sprint-69-K-Phab-and-the-Kurious-Koders-mmKay ;)

MBinder_WMF set the point value for this task to 5.Apr 18 2016, 4:12 PM

Change 275724 merged by jenkins-bot:
Allow lazy loading references via mobileview API

https://gerrit.wikimedia.org/r/275724

@MBinder_WMF: IMO there's still some work to be done here but it needs better explanation/more detail from me and more discussion with the team before we can proceed. This work is tracked by T133015. I'm happy enough with 275724 that this task should no longer block T129182 so I'm moving this to Done but not closing it.

Does this seem reasonable?

@phuedx I would suggest creating a new task or subtask to avoid any unnecessary confusion.

@phuedx I think @Jdlrobson suggestion is a good one. I'd comment in this ticket (or update the description) to say what the scope change is, and create another task for remaining work.

@phuedx I think @Jdlrobson suggestion is a good one. I'd comment in this ticket (or update the description) to say what the scope change is, and create another task for remaining work.

I already have. See T133015.

Jdlrobson updated the task description. (Show Details)
Jdlrobson added a subscriber: Mholloway.

That task doesn't really add anything that this task already says so rather than cause confusion I've merged it in.

Change 353546 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] WIP: ReferencesController

https://gerrit.wikimedia.org/r/353546

Change 353546 abandoned by Jdlrobson:
WIP: ReferencesController

Reason:
Not working on this right now

https://gerrit.wikimedia.org/r/353546

Jdlrobson removed the point value for this task.

There is no mobile.references module any more. This task likely needs a new estimation and discussion and will likely be simplified if we remove lazy loaded references feature.

Jdlrobson renamed this task from Refactor mobile.references module to Refactor mobile references code.Apr 23 2019, 1:16 AM
Jdlrobson added a subscriber: bmansurov.

Lazy loaded reference feature will be removed (see T222373)