Page MenuHomePhabricator

Refactor mobile references code
Closed, DeclinedPublic

Description

Currently, this task captures the conversation around the refactoring of the 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

Details

Related Gerrit Patches:
mediawiki/extensions/MobileFrontend : masterWIP: ReferencesController
mediawiki/extensions/MobileFrontend : masterAllow lazy loading references via mobileview API

Related Objects

StatusAssignedTask
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
DeclinedNone
OpenNone
Resolveddr0ptp4kt
DuplicateJhernandez
Duplicatedr0ptp4kt
OpenNone
ResolvedJdlrobson
DeclinedNone
ResolvedJdlrobson
DeclinedNone
DuplicateJdlrobson

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 :/)

jhobs triaged this task as Low priority.Mar 22 2016, 5:09 PM
phuedx updated the task description. (Show Details)Apr 8 2016, 9:56 AM

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.

(Minus controller)

Should this get a sprint tag on it?

phuedx updated the task description. (Show Details)Apr 11 2016, 5:07 PM
phuedx assigned this task to Jdlrobson.Apr 11 2016, 5:39 PM

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

phuedx updated the task description. (Show Details)Apr 19 2016, 8:19 AM

@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.

Restricted Application added a subscriber: TerraCodes. · View Herald TranscriptApr 19 2016, 3:29 PM

@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 removed Jdlrobson as the assignee of this task.Apr 10 2017, 7:26 PM
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.

phuedx updated the task description. (Show Details)May 12 2017, 12:34 PM

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 moved this task from Backlog to Tasks on the MobileFrontend board.Jul 13 2017, 5:55 PM
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.
Jdlrobson closed this task as Declined.Jul 17 2019, 4:16 PM

Lazy loaded reference feature will be removed (see T222373)