Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

Migrate Share Sheet to LibState #4007

Closed
jonalmeida opened this issue Jul 11, 2019 · 10 comments
Closed

Migrate Share Sheet to LibState #4007

jonalmeida opened this issue Jul 11, 2019 · 10 comments
Assignees
@Mugurell
Labels
E3 Estimation Point: average, 2 - 3 days eng:health Improve code health eng:qa:verified QA Verified help wanted Help wanted from a contributor. More complex than good first issue. P2 Upcoming release
Projects
A-C: Android Components Sprint Planning
Milestone
v2.0

Comments

@jonalmeida
Copy link
Contributor

jonalmeida commented Jul 11, 2019 •
edited by data-sync-user

We don't need most of the stateful stuff in the current ShareFragment so we can start on removing this.

┆Issue is synchronized with this Jira Task
@jonalmeida jonalmeida added eng:ready Ready for engineering eng:health Improve code health labels Jul 11, 2019
@jonalmeida jonalmeida self-assigned this Jul 11, 2019
@jonalmeida jonalmeida added this to Fenix Integration in A-C: Meta: Send Tab Components Jul 11, 2019
@boek boek added this to To be Triaged in Fenix Sprint Kanban via automation Jul 17, 2019
@boek boek moved this from To be Triaged to Prioritized Eng Backlog in Fenix Sprint Kanban Jul 17, 2019
@sblatz sblatz removed this from Prioritized Eng Backlog in Fenix Sprint Kanban Jul 19, 2019
@jonalmeida jonalmeida changed the title [Share] Migrate Share Sheet to LibState Migrate Share Sheet to LibState Jul 24, 2019
@jonalmeida
Copy link
Contributor Author

Differing this ticket until we have consensus in #4275.

@jonalmeida jonalmeida removed their assignment Jul 24, 2019
@jonalmeida jonalmeida removed this from Fenix Backlog in A-C: Meta: Send Tab Components Jul 24, 2019
@sblatz sblatz added the P5 Never, but will accept a patch label Jul 31, 2019
@sblatz
Copy link
Contributor

sblatz commented Jul 31, 2019

Labeling this as p5 until we have a decision :)

@liuche liuche removed the eng:ready Ready for engineering label Aug 7, 2019
@sblatz sblatz added P2 Upcoming release and removed P5 Never, but will accept a patch labels Aug 16, 2019
@sblatz sblatz added this to To be Triaged in Fenix Sprint Kanban via automation Aug 16, 2019
@sblatz
Copy link
Contributor

sblatz commented Aug 16, 2019

We're sticking with this custom sheet!

@sblatz sblatz moved this from To be Triaged to Prioritized Eng Backlog in Fenix Sprint Kanban Aug 16, 2019
@sblatz sblatz added the help wanted Help wanted from a contributor. More complex than good first issue. label Aug 16, 2019
@liuche liuche added the E3 Estimation Point: average, 2 - 3 days label Aug 16, 2019
@liuche liuche moved this from Prioritized Eng Backlog to Sprint Backlog in Fenix Sprint Kanban Aug 16, 2019
@boek boek added this to the v1.4 milestone Aug 16, 2019
@Mugurell Mugurell self-assigned this Aug 21, 2019
@liuche liuche moved this from Sprint Backlog to Prioritized Eng Backlog in Fenix Sprint Kanban Aug 21, 2019
@jonalmeida
Copy link
Contributor Author

jonalmeida commented Aug 21, 2019 •
edited

Engineering notes: the share sheet actually keeps no state at all, so this would really be a refactor to remove the old architecture stuff and follow the view/interactor patterns similar to other refactors.

Also consider taking out the old send tab code and make use of the SendTabUseCases which are much easier to deal with.

@jonalmeida
Copy link
Contributor Author

@Mugurell are you actively working on this? If you aren't planning to work on it next week, do you mind if I steal this then? :)

@Mugurell
Copy link
Contributor

@Mugurell are you actively working on this? If you aren't planning to work on it next week, do you mind if I steal this then? :)

Got sidetracked a little but finishing it up right now with some tests.

@Mugurell
Copy link
Contributor

Also consider taking out the old send tab code and make use of the SendTabUseCases which are much easier to deal with.

Looked into using SendTabUseCases but found a few issues in doing so:

@boek boek moved this from Prioritized Eng Backlog to In Progress in Fenix Sprint Kanban Aug 29, 2019
boek pushed a commit that referenced this issue Aug 30, 2019
@Mugurell @boek
For #4007 - Use camelCase for views referenced from code
5e19741 
As per #4341.
Also reformatted layouts to have a more consistent style.
Also refactored `AppShareRecyclerView` and `AccountDevicesShareRecyclerView` by
defining their LayoutManager in XML to reduce code complexity.
boek pushed a commit that referenced this issue Aug 30, 2019
@Mugurell @boek
For #4007 - Refactor AppShareView in standalone Share Views
0958701 
In an effort to respect the initial MVI architecture I've broken the
complex `AppShareView` in 3 separate Views
- `ShareCloseView`
- `ShareToAccountDevicesView`
- `ShareToAppsView`
They are standalone Views (extending LayoutContainer) which know nothing about
each other or their parent and so offer their container the possibility to
order or display them in any form later.
According to the lib-state contract they are only responsible to
- inflate themselves in their injected containerView
- render a certain state (to be added in later commits)
- delegate all user interaction to an associated Interactor
boek pushed a commit that referenced this issue Aug 30, 2019
@Mugurell @boek
For #4007 - ShareFragment will set the contained Views' state
16eba61 
ShareFragment which acts as a container would contain all business logic needed
for populating it's Views.
Data initialization should be done only once since the app state has no reason
to change after the ShareFragment is created and is done as soon as possible,
in onAttach().
Because of the expected short lifespan of this fragment, given the fact that
the state has no reason to change and we handle orientation changes ourselves
to keep things simple I didn't use a ViewModel to persist the state.
boek pushed a commit that referenced this issue Aug 30, 2019
@Mugurell @boek
For #4007 - Add ShareController for handling ShareFragment's business…
e165868 
… logic

`ShareController` defines a contract with all possible `ShareFragment`'s
behavior changes and comes with a default implementation -
`DefaultShareController`.
It is to be delegated by all `ShareFragment`s contained Views' Interactors
following any user interactions.
@boek boek modified the milestones: v1.4, v1.5 Aug 30, 2019
@boek boek added the eng:qa:needed QA Needed label Aug 30, 2019
@project-bot project-bot bot moved this from In Progress to Ready for QA in Fenix Sprint Kanban Aug 30, 2019
@boek
Copy link
Contributor

boek commented Aug 30, 2019

For QA: Refactor of the share sheet implementation. Please smoke test sharing to different apps and send tab 👍

@jonalmeida
Copy link
Contributor Author

jonalmeida commented Sep 1, 2019 •
edited

@Mugurell

it uses TabData which is not Parcelable. This is a requirement if we want it sent as an argument through Navigation arch component.

Feel free to map the TabData to a ShareTab if needed. The use cases give us better control of the send tab APIs for all the browser apps, so we should use them.

The current ShareTab also contains a sessionId while TabData doesn't. Not sure about it's importance and if it can be ignored.

At one point the sessionId may have been used, but I no longer see references to it so let's just nuke that from the data class.

@Mugurell Mugurell mentioned this issue Sep 2, 2019
Closed
@AndiAJ AndiAJ added eng:qa:verified QA Verified and removed eng:qa:needed QA Needed labels Sep 3, 2019
@project-bot project-bot bot moved this from Ready for QA to In Progress in Fenix Sprint Kanban Sep 3, 2019
@AndiAJ
Copy link
Collaborator

AndiAJ commented Sep 3, 2019

Hi, verified as fixed on Fenix 2.0.0-rc.1

Tested the following:

Tested the following:

Quick Action bar - Share button
Page menu - Page menu
Bookmarks (Single and multi-select sharing)
History (Single and multi-select sharing)
Share tabs from the tabs view
Share a tab from the main menu
Share all tabs in a collection
Share using context menus
Send tabs
Custom tabs

@AndiAJ AndiAJ closed this as completed Sep 3, 2019
@AndiAJ AndiAJ added this to 🏁 Done in A-C: Android Components Sprint Planning via automation Sep 3, 2019
Fenix Sprint Kanban automation moved this from In Progress to Done Sep 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@Mugurell Mugurell

Labels
E3 Estimation Point: average, 2 - 3 days eng:health Improve code health eng:qa:verified QA Verified help wanted Help wanted from a contributor. More complex than good first issue. P2 Upcoming release
Projects
No open projects
A-C: Android Components Sprint Planning
  
🏁 Done
Milestone
v2.0
Development

No branches or pull requests
6 participants
@liuche @boek @jonalmeida @sblatz @Mugurell @AndiAJ