Differential D21776

Implement new Header toolbar style for main view
ClosedPublic
Actions

Authored by ngraham on Jun 12 2019, 9:32 PM.

Details

Reviewers
mgallien
astippich
Group Reviewers
Elisa
VDG
Commits
R255:7160095af12f: Implement new Header toolbar style for main view
Summary

This patch implements the new header toolbar style for the main view.

Of note, I had to remove the subtitle and make the buttons icons-only toolbuttons.
If these are considered problems, what we could make do is always use the toolbar to
display the category name (rather than the album name etc) to make room for the
toolbuttons regaining their labels, then put the artist/album/etc names below inside
the content view itself, sort of like what I did in the Context view in D21771. That
would probably be best suited as material for a follow-up patch though since it would
entail porting to QQC2 to get toolbuttons that can have visible text.

Test Plan

Before, search tools collapsed:


After: search tools collapsed:

Before: search tools expanded and in use:


After: search tools expanded and in use:

Before: drilldown into an album, showing a list:


After: drilldown into an album, showing a list:

Diff Detail

Repository
R255 Elisa
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
ngraham requested review of this revision.Jun 12 2019, 9:32 PM
ngraham created this revision.
mgallien added a comment.Jun 14 2019, 9:13 PM

Thanks for your work.

Sorry for the delay.
Due to me needed to rebuild a few frameworks and trying to fix a few bugs bothering me, I have not a detailed feedback.

The top level views look good to me.

The other views, I am not convinced by the fact that we would remove information. I would even prefer to go the other way around (like providing more data on an artist, album ...).

I would be pretty happy to get the top level views to adopt this new design. Would it be possible to somehow have either a different headers depending on the view (and its level) ?

Another solution could be to delay working on the album, artist, ... views.

What do you think ?

ngraham added a comment.Jun 14 2019, 9:50 PM

I was also not sure about removing information. An idea I had was to make the actual toolbar itself very minimal and only show the category name + toolbar buttons. Then all the fancy information could be below that, visible in the view itself, separated in a pretty way like how I did with the context view.

astippich added a subscriber: astippich.Jun 19 2019, 5:43 PM

While you're at it, could you port the remaining actions and buttons to qqc2 using the FlatButtonWithTooltip? If you override the "display" property to "AbstractButton.TextBesideIcon" you should also be able to get the text

ngraham added a comment.Jun 20 2019, 7:32 AM
In D21776#482068, @astippich wrote:

While you're at it, could you port the remaining actions and buttons to qqc2 using the FlatButtonWithTooltip? If you override the "display" property to "AbstractButton.TextBesideIcon" you should also be able to get the text

Yeah, I was planning to do that afterwards, and get them all at once in a single patch.

ngraham added a comment.Jun 20 2019, 7:33 AM
In D21776#479886, @ngraham wrote:

I was also not sure about removing information. An idea I had was to make the actual toolbar itself very minimal and only show the category name + toolbar buttons. Then all the fancy information could be below that, visible in the view itself, separated in a pretty way like how I did with the context view.

Anyway, to move forward, I'm interested in hearing opinions on the above ^^^

Elisa or VDG people?

mgallien added a comment.Jun 20 2019, 7:36 AM
In D21776#482189, @ngraham wrote:
In D21776#479886, @ngraham wrote:

I was also not sure about removing information. An idea I had was to make the actual toolbar itself very minimal and only show the category name + toolbar buttons. Then all the fancy information could be below that, visible in the view itself, separated in a pretty way like how I did with the context view.

Anyway, to move forward, I'm interested in hearing opinions on the above ^^^

Elisa or VDG people?

Sorry for my late reply. I am all for it. I am sure we will be able to push this forward.

ngraham planned changes to this revision.Jun 20 2019, 7:36 AM

OK, great!

ngraham updated this revision to Diff 60138.Jun 20 2019, 3:06 PM

Re-add secondary title to new toolbar; don't lose anything

ngraham edited the test plan for this revision. (Show Details)Jun 20 2019, 3:07 PM

I decided to put the secondaryTitle text back into the new toolbar rather than moving stuff below it. This way it remains much more compact, and all the same information is still there.

ngraham planned changes to this revision.Jun 20 2019, 3:16 PM

Tests are broken

ngraham updated this revision to Diff 60146.Jun 20 2019, 3:58 PM

Fix tests; ready for review

mgallien added a comment.Jun 24 2019, 9:02 PM

It has really evolved to be something very desirable. Congratulations on the very good work.

I noticed one problem when testing:
qrc:/qml/HeaderFooterToolbar.qml:64:9: QML RowLayout: Possible anchor loop detected on vertical anchor.
When trying to open an album from an artist and a genre.

ngraham added a comment.Jun 24 2019, 9:43 PM
In D21776#486316, @mgallien wrote:

It has really evolved to be something very desirable. Congratulations on the very good work.

I noticed one problem when testing:
qrc:/qml/HeaderFooterToolbar.qml:64:9: QML RowLayout: Possible anchor loop detected on vertical anchor.
When trying to open an album from an artist and a genre.

Thank you very much for the encouragement. It's very... encouraging! :)

I will take a look at the anchor loop issue tomorrow morning.

mikeroyal added a subscriber: mikeroyal.Jun 24 2019, 10:41 PM

@ngraham, I agree with you on the very minimal and only show the category name + toolbar buttons.

ngraham added a comment.Jun 25 2019, 8:54 AM

@mgallien I am not able to reproduce the anchor loop warning. The new code I've added actually doesn't even set any anchors on the child items of the HeaderFooterToolbar components.

mgallien added a comment.Jun 25 2019, 9:21 AM
In D21776#486386, @ngraham wrote:

@mgallien I am not able to reproduce the anchor loop warning. The new code I've added actually doesn't even set any anchors on the child items of the HeaderFooterToolbar components.

I will have another look tonight and report here. Sorry

mgallien added a comment.Jun 25 2019, 9:05 PM
In D21776#486408, @mgallien wrote:
In D21776#486386, @ngraham wrote:

@mgallien I am not able to reproduce the anchor loop warning. The new code I've added actually doesn't even set any anchors on the child items of the HeaderFooterToolbar components.

I will have another look tonight and report here. Sorry

I am still having the problem when navigating this way:
Genre -> Artist -> Album

I am using Qt 5.11.3 (default version on my distribution). Maybe the difference is related to that.

ngraham added a comment.Jun 25 2019, 9:12 PM

Hmm, could be. I'm on 5.12.3.

astippich added a comment.Jun 29 2019, 2:09 PM

Could also be related to the frameworks version

ngraham added a comment.Jun 30 2019, 3:19 PM
In D21776#488157, @astippich wrote:

Could also be related to the frameworks version

@mgallien does it still happen with Frameworks 5.60? Since we're bumping the dependency to that, it might make sense to test with it as the baseline.

astippich added a comment.Jul 3 2019, 9:47 AM

FYI, I am not able to get this binding loop. I am on frameworks 5.59 and Qt 5.12.2
One small nitpick inline. Additionally, the Back Navigation button was previously hidden for the top views, now it is just disabled. I find it a little bit hard to distinguish between both states.

src/qml/NavigationActionBar.qml
169

Undefined will give an error. Did you mean Font.Normal?

mgallien added a comment.Jul 3 2019, 7:33 PM
In D21776#490023, @astippich wrote:

FYI, I am not able to get this binding loop. I am on frameworks 5.59 and Qt 5.12.2
One small nitpick inline. Additionally, the Back Navigation button was previously hidden for the top views, now it is just disabled. I find it a little bit hard to distinguish between both states.

I am busy with real life (mainly due to weather) and will not be able to review anything before Sunday at best. Sorry for that.

@astippich, if you are able to test, you can also accept it instead of letting it wait forever.

src/qml/NavigationActionBar.qml
169

Generate a warning when assigning undefined, Font.normal should be better.

ngraham updated this revision to Diff 61217.Jul 5 2019, 2:13 PM
ngraham marked 2 inline comments as done.
  • Fix "undefined" not being defined (lol)
  • Don't show a pointless disabled Back button for top-level views
astippich accepted this revision.Jul 5 2019, 5:50 PM
This revision is now accepted and ready to land.Jul 5 2019, 5:50 PM
Closed by commit R255:7160095af12f: Implement new Header toolbar style for main view (authored by ngraham). · Explain WhyJul 7 2019, 1:35 AM
This revision was automatically updated to reflect the committed changes.
ngraham added a comment.Jul 9 2019, 10:17 PM

Now that this had landed, I can reproducibly reproduce the anchor loop issue that @mgallien saw when navigating from Genre → Artist → album.

Fixed with https://commits.kde.org/elisa/1906e6e9d22102d7d491253ccb240281f13a438b.

Sorry about that!

mgallien added a comment.Jul 10 2019, 6:00 AM
In D21776#493280, @ngraham wrote:

Now that this had landed, I can reproducibly reproduce the anchor loop issue that @mgallien saw when navigating from Genre → Artist → album.

Fixed with https://commits.kde.org/elisa/1906e6e9d22102d7d491253ccb240281f13a438b.

Sorry about that!

No problem and thanks for your work

Revision Contents
Changeset List

Diff 61261

View Options

src/qml/BaseTheme.qml

Loading...
View Options

src/qml/FileBrowserView.qml

Loading...
View Options

src/qml/GridBrowserView.qml

Loading...
View Options

src/qml/HeaderFooterToolbar.qml

Loading...
View Options

src/qml/ListBrowserView.qml

Loading...
View Options

src/qml/NavigationActionBar.qml

Loading...