Page MenuHomePhabricator

Refactor MobileFormatter class: Use IMobileTransform for all transformations
Closed, ResolvedPublic5 Estimated Story Points

Description

During the lead paragraph work we began a refactor of the MobileFormatter class for best testability and readability.

Code coverage of the new transforms is good:
https://doc.wikimedia.org/cover-extensions/MobileFrontend/transforms/index.html
https://doc.wikimedia.org/cover-extensions/MobileFrontend/MobileFormatter.php.html

Work (complete)

Sign off steps

  • MobileContext contains many methods which should be migrated to the MobileFormatter - for example, [get|set]ContentTransformations. Review and port as appropriate. Create tasks.
  • Review remaining transforms and their purpose (are they redundant?) . Create tasks.

Event Timeline

Jdlrobson triaged this task as Medium priority.Jul 10 2018, 9:36 PM

Change 448415 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Separate the LazyLoadedImages transform from MobileFormatter

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

Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)

Change 448415 merged by Pmiazga:
[mediawiki/extensions/MobileFrontend@master] Separate the LazyLoadedImages transform from MobileFormatter

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

Jdlrobson edited projects, added Web-Team-Backlog; removed Web-Team-Backlog (Tracking).
Jdlrobson added subscribers: Peter.ovchyn, ovasileva.

@ovasileva Possible an important piece of maintenance that @Peter.ovchyn might be able to help with.

Jdlrobson renamed this task from [EPIC] Complete the refactor of the MobileFormatter class to Complete the refactor of the MobileFormatter class.Aug 3 2020, 7:12 PM
Jdlrobson removed a project: Epic.

@ovasileva Possible an important piece of maintenance that @Peter.ovchyn might be able to help with.

Sounds good to me

Change 620716 had a related patch set uploaded (by Peter.ovchyn; owner: Peter.ovchyn):
[mediawiki/extensions/MobileFrontend@master] Move move doRemoveImages to class RemoveImageTransform

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

Peter.ovchyn updated the task description. (Show Details)
Peter.ovchyn updated the task description. (Show Details)

Change 621981 had a related patch set uploaded (by Peter.ovchyn; owner: Peter.ovchyn):
[mediawiki/extensions/MobileFrontend@master] WIP: Move makeSections to MakeSectionsTransform class

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

Change 620716 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Move doRemoveImages to class RemoveImagesTransform

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

Even though this task is done (formally), there's a some of stuff to do next. Dropping this here to form into tasks in the future:

1. Extract makeHeadingsEditable to MakeHeadingsEditableTransfrom class.
2. Move creating of RemoveImagesTransform and MakeSectionsTransform to MobileFormatter::__constructor

At the moment all transforms are created right "in-place" in MobileFormatter::filterContent
This demands for us to keep some variables like MobileFormatter::$title, MobileFormatter::$topHeadingTags etc, only to create Transformers.
If we move creating them to MobileFormatter::__constructor we could avoid that.

At the moment we are blocked by MobileFormatter::$scriptsEnabled that is changed in MobileFormatter::disableScripts.

3. Move conditional to MobileFormatter:: __construct and use polymorphism.

At the moment there's a conditional behavior that depends on some settings like:

if ( $this->removeMedia ) {
    $removeImagesTransform = new RemoveImagesTransform();
    /** @phan-suppress-next-line PhanTypeMismatchArgument DOMNode vs. DOMElement */
    $removeImagesTransform->apply( $doc->getElementsByTagName( 'body' )->item( 0 ) );
}

This demands from us keeping them in class. Once 1 and 2 are done we could simplify this by moving conditions to constructor and use NoTransfrom when we don't need transformation like:

public function __construct(...) {
    if ( $this->removeMedia ) {
        $this->removeImagesTransform = new RemoveImagesTransform();
    } else {
        $this->removeImagesTransform = NoTransform::getInstance();
   }
}
// Then we can call without and conditional so we don't need to keep variable:

public function filterContent( ... ) {
...
        /** @phan-suppress-next-line PhanTypeMismatchArgument DOMNode vs. DOMElement */
        $removeImagesTransform->apply( $doc->getElementsByTagName( 'body' )->item( 0 ) );
...
}
4. Replace Transformers with array:

when 1-3 are don, we will have code like that:

class MobileFormatter extends HtmlFormatter {
    private $removeImagesTransform;
    private $makeHeadingsEditable; 
...
and then..
    public function __construct( ... ) {
       if ( $removeMedia ) {
          $this-> removeImagesTransform = NoTransform::getInstance();
       } else {
          $this->removeImagesTransform = new RemoveImagesTransform();
       }
      // The same as above for other transformers. 
     ...
    }
...
    public function filterContent( ... ) {
        $this->removeImagesTransform->apply();
        $this->makeSectionsTransform->apply();
        ....
    }
}

All above could be replaced with array like:

class MobileFormatter extends HtmlFormatter {
    private $transformers = [];
...
and then..
    public function __construct( ... ) {
       if ( !$removeMedia ) {
          $this->transformers []= new RemoveImagesTransform();
       }
      // The same as above for other transformers. 
     ...
    }
...
    public function filterContent( ... ) {
        foreach ( $this->removeImagesTransform as $transformer) {
            $transformer->apply();
        }
        ....
    }
}
5. Flatten the transformers

At the moment MobileSectionTransform contains nested transformers that transform sections: LazyImageTransform and MoveLeadParagraphTransform
It would be good to extract them out of the MobileSectionTransform and use equally to others. It might be a bit tricky and it will demand working with OOP hierarchies (or like that) but still possible.

After steps 1-5 we'll have 100% generic solution that will work equally with all transformers. All that will allow us change any this piece of functionality quickly. Not sure about safety as in certain ways order matters.

Jdlrobson updated the task description. (Show Details)

Hey @Peter.ovchyn I had a read through T186823#6409592 and mostly agree with the approach, and do agree with the direction. I think MobileFormatter::filterContent got conflated with HtmlFormatter::filterContent and the execution of transforms should be separated from that method as really it's a function for removing content not transforming. I began https://gerrit.wikimedia.org/r/c/mediawiki/extensions/MobileFrontend/+/622866 to explore what I think the direction should look like.

I think 1 "1. Extract makeHeadingsEditable to MakeHeadingsEditableTransfrom class." should be done as part of this work, however. Is that okay with you? Let's call it SubHeadingTransform. I'm sorry I missed it in the original task description.

Change 623415 had a related patch set uploaded (by Peter.ovchyn; owner: Peter.ovchyn):
[mediawiki/extensions/MobileFrontend@master] Move makeHeadingsEditable to SubHeadingTransform class

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

Change 621981 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Move makeSections to MakeSectionsTransform class

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

Change 623415 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Move makeHeadingsEditable to SubHeadingTransform class

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

I'll make some follow up tasks per @Peter.ovchyn excellent write up in T186823#6409592

Jdlrobson renamed this task from Complete the refactor of the MobileFormatter class to Refactor MobileFormatter class: Use IMobileTransform for all transformations.Sep 1 2020, 4:40 PM