1143928 - Refactor Deck and Panel classes to reduce code duplication

Status: RESOLVED FIXED

(Testing :: Firefox UI Tests, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Some of our current ui libraries are using decks and panels. The implementation is kinda close, so we should reduce the duplication and refactor both classes out for a more general usage.

https://github.com/mozilla/firefox-ui-tests/blob/master/firefox_puppeteer/ui/about_window/deck.py

https://github.com/mozilla/firefox-ui-tests/blob/master/firefox_puppeteer/ui/pageinfo/deck.py

Comment 1

[utvar]

Whimboo, I was thinking of trying this bug next. Do you want all the classes in both ui/pageinfo/deck.py and ui/about_window/deck.py refactored out and placed directly in ui/deck.py, or do you only want the Deck and Panel classes refactored out? How about the empty classes?

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

We should only refactor out the basics but no code related to specific deck implementations which are only present for a single deck instance. That would also include the list of panels (also empty classes) which will be different for each deck instance.

Let me know if you need more information to get started. I will happily provide those.

Comment 3

[utvar]

I can't seem to assign myself this bug; should I ask you to do that now? Or after I've created a PR? The links you provided in your original comment seem to be from the outdated master branch, so I guess you would like me to create the PR against mozilla-central?

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Given that you already contributed some helpful patches I can already assign this bug to you. Regarding your question, yes that is the branch we have to create the PR against.

Comment 5

[utvar]

Attachment: Github pull request

Whimboo, is this close to what you had in mind? I started by refactoring out Panel, and wanted to check to see if I have the right idea before proceeding with Deck. I hesitate to refactor out the tab property, might that cause problems?

Also, it seems the Travis build failed due to a time out -- I'm not sure how to tell if my code is responsible for that, but the tests did pass locally.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8689953 [details] [review]
Github pull request

The panels are closely connected to the deck widget. So I don't think factoring them out into their own module would make that much of sense. Best really is to create a new deck.py module and get both generic classes for decks and panels moved over. Then consumers of the deck would only have to make their own customizations.

Comment 7

[utvar]

Comment on attachment 8689953 [details] [review]
Github pull request

I have refactored out Panel, but Deck remains a mystery. It may take some time to figure out Deck, so I thought I'd submit this much for review now.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8689953 [details] [review]
Github pull request

Utvar, sadly we had to merge the python_packages branch into mozilla-central, which actually moved all the files to a new location (one level deeper). So this PR doesn't apply anymore. Would you mind to apply those changes again after you did a git rebase? Thanks.

Comment 9

[utvar]

Comment on attachment 8689953 [details] [review]
Github pull request

I have rebased and moved the ui/deck.py file, so I think the PR is okay again. See GitHub for comments.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8689953 [details] [review]
Github pull request

This PR got landed on the github repo:

https://github.com/mozilla/firefox-ui-tests/commit/023fb06a01b86d56363a0dc46c2317a79aa09570

I will have to port this to hg.mozilla.org later today. 

Utvar, will you be able to continue the work on this bug for the remaining items?

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch for hg.mo

Updated patch with no code changes except updated paths for hg.mo

Comment 12

[utvar]

(In reply to Henrik Skupin (:whimboo) from comment #10)
> Comment on attachment 8689953 [details] [review]
> Github pull request
> 
> This PR got landed on the github repo:
> 
> https://github.com/mozilla/firefox-ui-tests/commit/
> 023fb06a01b86d56363a0dc46c2317a79aa09570
> 
> I will have to port this to hg.mozilla.org later today. 
> 
> Utvar, will you be able to continue the work on this bug for the remaining
> items?

I have been fairly busy, so it might take me some time to learn mercurial and the new repo structure. If you prefer to leave the bug unassigned that's fine, and I could try to finish it up at some point, if nobody else has grabbed it. Thanks...

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

No need to learn Mercurial. Git can still be used via the git-cinnabar extension. So for now I will move this bug back to unassigned in case someone else wants to continue. Otherwise pick it up when you are free again. Thanks for letting me know!

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a4315513655c

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a4315513655c

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Wow, a long outstanding bug. I think with the code as landed lets mark it as fixed. If we need more work done we can file a new bug, but I actually cannot remember anymore, what's left to do here.