Merge lp:~ahayzen/music-app/convergence-tabs-with-sidebar-01 into lp:music-app
| Status: | Merged | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Approved by: | Victor Thompson on 2016-04-06 | ||||||||
| Approved revision: | 984 | ||||||||
| Merged at revision: | 998 | ||||||||
| Proposed branch: | lp:~ahayzen/music-app/convergence-tabs-with-sidebar-01 | ||||||||
| Merge into: | lp:music-app | ||||||||
| Diff against target: |
2029 lines (+1077/-432) 26 files modified
app/components/BlurredBackground.qml (+3/-1) app/components/Flickables/MusicGridView.qml (+0/-10) app/components/HeadState/EmptyHeadState.qml (+56/-0) app/components/HeadState/MultiSelectHeadState.qml (+89/-76) app/components/HeadState/PlaylistHeadState.qml (+78/-0) app/components/HeadState/PlaylistsHeadState.qml (+54/-20) app/components/HeadState/QueueHeadState.qml (+89/-0) app/components/HeadState/SearchHeadState.qml (+59/-46) app/components/HeadState/SearchableHeadState.qml (+45/-8) app/components/MusicPage.qml (+39/-0) app/components/NowPlayingFullView.qml (+76/-68) app/components/NowPlayingSidebar.qml (+134/-0) app/components/NowPlayingToolbar.qml (+18/-14) app/components/Queue.qml (+5/-1) app/music-app.qml (+101/-23) app/ui/AddToPlaylist.qml (+5/-2) app/ui/Albums.qml (+5/-2) app/ui/ArtistView.qml (+12/-2) app/ui/Artists.qml (+5/-2) app/ui/ContentHubExport.qml (+34/-26) app/ui/Genres.qml (+5/-2) app/ui/NowPlaying.qml (+113/-77) app/ui/Playlists.qml (+5/-2) app/ui/Recent.qml (+41/-15) app/ui/SongsView.qml (+5/-35) debian/changelog (+1/-0) |
||||||||
| To merge this branch: | bzr merge lp:~ahayzen/music-app/convergence-tabs-with-sidebar-01 | ||||||||
| Related bugs: |
|
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Victor Thompson | 2016-02-16 | Approve on 2016-04-06 | |
| Jenkins Bot | continuous-integration | Approve on 2016-04-06 | |
|
Review via email:
|
|||
Commit Message
* Implement convergent mode with now playing and queue as a sidebar
Description of the Change
* Implement convergent mode with now playing and queue as a sidebar
Known Issues:
* When the app is starting and is wide enough for wideAspect, a black box appears where the async loader ends up (this happens for all apps seems to be they are launched in portrait and resized too late) [bug 1548096]
PASSED: Continuous integration, rev:976
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
- 977. By Andrew Hayzen on 2016-03-04
-
* Add missing file and set dividor colour
PASSED: Continuous integration, rev:977
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
- 978. By Andrew Hayzen on 2016-03-04
-
* Set divider to be darker not lighter
PASSED: Continuous integration, rev:978
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
- 979. By Andrew Hayzen on 2016-03-04
-
* Set flickable on fallback PageHeader
PASSED: Continuous integration, rev:979
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
| Victor Thompson (vthompson) wrote : | # |
Added some inline code issues/questions. Will try to do some functional testing as well tonight/tomorrow.
| Victor Thompson (vthompson) wrote : | # |
I've noticed a few things while testing this mp:
1. The top of the queue is obscured by the header sections.
2. Only applicable to the new SDK in silo 50, but the header sections seem quite a bit larger.
3. Only applicable to the new SDK in silo 50, but when I select Queue, then select Full view the header disappears.
- 980. By Andrew Hayzen on 2016-03-07
-
* Various fixes
| Andrew Hayzen (ahayzen) wrote : | # |
Issues:
1) Was there a specific reason to do this? Did you plan on referencing it elsewhere?
2) It'd be tiny, but since we replicate this all over the place it would be nice to make a MusicStyleHints or perhaps put the "1.1" value in the Style.qml file. Actually, maybe create a base MusicState for all these to implement?
3) This bug was fixed in OTA9, you can leave the workaround in if you want, but otherwise we should put together an MP to remove it.
4) Move this commented out code? Or will it be needed when bug is resolved?
5) This is a nitpick, but when we have consecutive updates, let's do 2013-2016, etc. You can leave it as-is for this MP though if you'd like.
6) I know we haven't been consistent, but we should avoid hardcoding this value all the time. Consider moving it to Style.qml
7) Please add parens to clarify the order of operations.
8) Does this deserve it's own component at all? I'd kind of argue that it's not necessary, but having a MusicTabActions component might be "nice" since this logic is rather simple.
9) Move below fill? Occurs a few times in various files.
10) The top of the queue is obscured by the header sections.
11) Only applicable to the new SDK in silo 50, but the header sections seem quite a bit larger.
12) Only applicable to the new SDK in silo 50, but when I select Queue, then select Full view the header disappears.
Resolutions:
1) This is required so that the sidebar can change the colour
2, 6) Styling stuff, I'd like to move to using proper themeing ASAP, so is it best just to leave hardcoded for now?
3) Removed code, please test
4) Fixed
5) I was told somewhere else it is best to specify all the years, and reduces confusion if someone then contributes in 2018 but not 2017 (as that'd have to be 2013-2016, 2018 not 2013-2018). But happy to change if that is what people want.
7) Fixed
8) I'm not sure, as in that component it'd have to reference the id tabs, also I was wondering in an additional MP if I could declare the tabs from a model with repeaters, which would then simplify this code alot.
9) Fixed
10) Fixed
11) "As per design", will probably be the response from upstream
12) Investigating... something weird going on :-)
PASSED: Continuous integration, rev:980
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
| Victor Thompson (vthompson) wrote : | # |
I can still reproduce #12 while on the new silo #50.
| Victor Thompson (vthompson) wrote : | # |
Figuring out why issue #12 is happening should be a priority so we can provide these features to early users of the tablet device.
- 981. By Andrew Hayzen on 2016-03-28
-
* Switch to using extension: Sections {} rather than sections: {} as that is deprecated
| Andrew Hayzen (ahayzen) wrote : | # |
Please retest #12, I've been unable to reproduce this seen moving to extension: Sections {} (as sections: {} will be deprecated).
FAILED: Continuous integration, rev:981
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
- 982. By Andrew Hayzen on 2016-03-28
-
* Merge of trunk
* Fixes for SongView to use PageHeader instead of PageHeadState
PASSED: Continuous integration, rev:982
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
- 983. By Andrew Hayzen on 2016-03-28
-
* Removal of the last PageHeadState to PageHeader
| Andrew Hayzen (ahayzen) wrote : | # |
Please test all headers as I found a few that hadn't been converted and therefore were broken.
PASSED: Continuous integration, rev:983
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
| Victor Thompson (vthompson) wrote : | # |
:( I can still reproduce the header bug. Was there anything that landed in rc-proposed that I'll need to test with (I'm still on a silo)?
| Victor Thompson (vthompson) wrote : | # |
Also, it seems like maybe the bug occurs more often when you have a large queue.
- 984. By Andrew Hayzen on 2016-04-06
-
* Create a separate page head state for FullView to get around flickable changing causing header to disappear
PASSED: Continuous integration, rev:984
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
| Victor Thompson (vthompson) wrote : | # |
Ok, the additional changes look good and fix the issue. Sometimes when I change orientations the header hides but can be re-shown. lgtm!
(but we should still try to track down what's going on)


Here are a few screenshots: http:// imgur.com/ a/sOqGU
I think the sidebar needs the most work if you compare it to the spec. However, the header also seems to differ in color?