Refactor the Global ToolBar concept
ClosedPublic

Authored by mart on Jun 21 2018, 3:55 PM.

Details

Summary

The recomended toolbar to be used for kirigami applications used to need to be explicitly instantiated, and had an internal flickable that tried hard to be synchronized with the main PageRow.
but this is pretty much impossible to have it glitch-free, it will there always be an edge case in which the two flickables gets out of sync, which is a strong sign of bad architecture.

Page toolbars are now moved on top of Page itself, so on the same Flickable as the PageRow itself. (a global header is still used when we are in breadcrumbs mode)
Is now also now possible to have multiple instances of PageRows, each one with its own globalToolBar. So, it's now also possible for AbstractApplicationHeader (and any subclass) to trach a different PageRow than the global one in ApplicationWindow.

also, there have been requests to be possible while keeping the default toolbar, have a custom global one that goes on top of it (ApplicationWindow's header item)

also, i wanted to have a way to make it possible to switch on the fly between a mobile-like and a desktop-like toolbar for further convergence plans

in the future, it will be possible for pages to put their own component to replace the default toolbar

CCBUG: 395455

Test Plan

tested all possible combinations of both the new way and the legacy compatibility mode.

ApplicationHeader and ToolBarApplicationHeader still work if used explicitly.

The patch shouldn't have significant visible UI changes.

there is still an explicit FIXME tough i want to do it in a different turn, as would need a new component

Diff Detail

Repository
R169 Kirigami
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
mart created this revision.Jun 21 2018, 3:55 PM
Restricted Application added a project: Kirigami. · View Herald TranscriptJun 21 2018, 3:55 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
mart requested review of this revision.Jun 21 2018, 3:55 PM
mart updated this revision to Diff 36472.Jun 21 2018, 3:56 PM
  • deprecate ToolbarApplicationHeader

Please can you add a description and check that the changes here are all relevant to that.

mart added a comment.Jun 21 2018, 4:08 PM

Please can you add a description and check that the changes here are all relevant to that.

sure, it was a screwup of the arc tool :)

mart retitled this revision from first prototype of moval of handles into PageRow to Refactor the Global ToolBar concept.Jun 21 2018, 4:18 PM
mart edited the summary of this revision. (Show Details)
mart edited the test plan for this revision. (Show Details)
mart added a reviewer: Kirigami.

Toolbar behavior in Desktop mode

Toolbar behavior in Mobile mode, including testing of features that were already there but kinda broken

ngraham added inline comments.
src/controls/ToolBarApplicationHeader.qml
42

onCompleted

IlyaBizyaev added inline comments.
src/controls/ToolBarApplicationHeader.qml
42

A typo :)

I don't know if it was supposed to, but FWIW this does not fix https://bugs.kde.org/show_bug.cgi?id=395455.

ngraham requested changes to this revision.Jun 21 2018, 6:37 PM

Also, this regresses the Install button width in Discover, and other main action buttons, which you can see in your screen recording. They're now very narrow and don't have enough side padding.

This revision now requires changes to proceed.Jun 21 2018, 6:37 PM
mart added a comment.EditedJun 22 2018, 9:04 AM

Also, this regresses the Install button width in Discover, and other main action buttons, which you can see in your screen recording. They're now very narrow and don't have enough side padding.

can you provide a screenshot just to track the evolution all here?

mart added a comment.EditedJun 22 2018, 9:07 AM

I don't know if it was supposed to, but FWIW this does not fix https://bugs.kde.org/show_bug.cgi?id=395455.

yes, it should fix that issue (only if ToolBarApplicationHeader is removed so the internal automatic thing is used instead tough)

mart edited the summary of this revision. (Show Details)Jun 22 2018, 9:07 AM
mart added a comment.Jun 22 2018, 9:21 AM

reproduced, adding here a comparison for recording prurposes
before the patch:


after the patch:

so indeed size hints of actiontoolbuttons changed a bit (before were indeed a bit too much tough) will tweak

mart updated this revision to Diff 36502.Jun 22 2018, 9:35 AM
mart marked 2 inline comments as done.
  • adress comments
ngraham resigned from this revision.Jun 22 2018, 1:01 PM

Better now, thanks!

What exactly do we have to do in Discover to make this patch fix https://bugs.kde.org/show_bug.cgi?id=395455?

mart added a comment.Jun 22 2018, 1:08 PM

Better now, thanks!

What exactly do we have to do in Discover to make this patch fix https://bugs.kde.org/show_bug.cgi?id=395455?

remove the line header: ToolBarApplicationHeader{} from the main file, that's it

Thanks, that seems to fix the bug!

However, it also introduces another issue: there's now a bunch of whitespace on the left of the toolbar:

apol added a subscriber: apol.Jun 22 2018, 3:23 PM
apol added inline comments.
examples/gallerydata/contents/ui/DesktopExampleApp.qml
28–29

Why is this commented?

examples/gallerydata/contents/ui/ExampleApp.qml
28–29

why the comment?

mart added inline comments.Jun 25 2018, 11:19 AM
examples/gallerydata/contents/ui/DesktopExampleApp.qml
28–29

old leftover, completely removing it now

mart updated this revision to Diff 36635.Jun 25 2018, 12:27 PM
  • remove dead code
  • check for handle visibility

Thanks, that fixes things in Discover in conjunction with D13663. There's only one remaining small visual issue: the whitespace to the left of the navigation is now too small, as seen in this screenshot:

mart added a comment.Jun 25 2018, 3:02 PM

Thanks, that fixes things in Discover in conjunction with D13663. There's only one remaining small visual issue: the whitespace to the left of the navigation is now too small, as seen in this screenshot:

right, of course for the first item the layout spacing isn't applied, will fix

mart updated this revision to Diff 36670.Jun 26 2018, 9:05 AM
  • fix spacing when there is no handle
mart added a comment.Jun 26 2018, 9:18 AM
In D13663#282780, @mart wrote:

right, of course for the first item the layout spacing isn't applied, will fix

spacing of the first button should be fixed

ngraham accepted this revision.Jun 26 2018, 12:47 PM

Thanks! I tried to break this in a bunch of ways but couldn't. Discover now feels a bit snappier, too.

Looks good from a visual and behavioral perspective now. No comment on the code.

This revision is now accepted and ready to land.Jun 26 2018, 12:47 PM
mart added a comment.Jun 26 2018, 1:28 PM

Thanks! I tried to break this in a bunch of ways but couldn't. Discover now feels a bit snappier, too.

Looks good from a visual and behavioral perspective now. No comment on the code.

last thing as retrocompatibility is important: can you confirm that it doesn't cause issues on an older discover (which is not patched to use the new toolbar)?

Yep, no regressions with an unpatched Discover (though obviously the bug is not fixed).

ngraham edited the summary of this revision. (Show Details)Jun 26 2018, 1:42 PM
mart updated this revision to Diff 36685.Jun 26 2018, 1:55 PM
  • use a binding
mart updated this revision to Diff 36686.Jun 26 2018, 2:21 PM
  • fix handle y
This revision was automatically updated to reflect the committed changes.