Improve on-screen help text for rectangular selection
ClosedPublic

Authored by rkflx on Apr 30 2018, 4:58 PM.

Details

Summary

D11599 adds a magnifier to the rectangular region selection screen, but
the corresponding help text on how to toggle the magnifier temporarily
is a bit hidden: Most users won't discover a mouse-over help text on a
checkbox in the config dialog.

To make the feature easier to notice, the corresponding text is added to
the on-screen help. As a prerequisite, the very long line of text is
first broken up into a more tabular style.

The second help text is also broken up and reworded. Both border styles are
adapted to slightly rounded corners.

Setting the default visibility of the help texts to false avoids
showing them initially for a brief moment.

Test Plan

spectacle -r, observe help texts with Remember selected area set
to on and also to off.

Before:

After:

Diff Detail

Repository
R166 Spectacle
Branch
better-selection-help-texts
Lint
No Linters Available
Unit
No Unit Test Coverage
rkflx requested review of this revision.Apr 30 2018, 4:58 PM
rkflx created this revision.
rkflx added a subscriber: ngraham.Apr 30 2018, 4:58 PM

@ngraham My first QML patch, please don't laugh but point out any improvements I could make to it :D

Also let me know if I shouldn't change the font size. I'm not sure yet about the compromise between making the text consistent and understated while also keeping it visible enough on large screens.

rkflx updated this revision to Diff 33352.Apr 30 2018, 5:04 PM

Remove unintentional newline

This comment was removed by ngraham.
ngraham added a comment.EditedMay 1 2018, 2:11 AM

Thanks for the patch! Your changes don't look bad at all, but the original code is in dire need of cleanup, and this may be a good opportunity to do some of that for the code you're touching. Specifically...

  • Use Label instead of Text: it respects the system font and size.
  • Optional, but desirable: use Kirigami.Heading instead of Label or Text: it's like Label, but has fancy logic to allow you to increase or decrease the size in a way that respects the user's chosen font size.
  • Optional, but desirable: use Kirigami.InlineMessage` instead of rolling our own visually-similar clone here. This is basically what InlineMessage was created for.

The two optional items will require adding Kirigami as a dependency, but more and more existing QWidgets-based KDE apps are eventually going to have to do this anyway when they gain QML-based UIs, so maybe we should bite the bullet and do it now. Kirigami adds a lot of fancy UI elements that make jobs like this a lot easier.

src/QuickEditor/EditorRoot.qml
281

How come?

rkflx added a comment.May 1 2018, 8:42 AM
  • Optional, but desirable: use Kirigami.Heading instead of Label or Text: it's like Label, but has fancy logic to allow you to increase or decrease the size in a way that respects the user's chosen font size.
  • Optional, but desirable: use Kirigami.InlineMessage` instead of rolling our own visually-similar clone here. This is basically what InlineMessage was created for.

TBH, I'd be in favour of replacing the QML parts entirely given the performance penalty of the current implementation (or at least rewrite everything in a way this is not an issue anymore). With that I specifically mean startup performance (which despite the QML cache is still pretty bad) as well as input lag when adjusting the rectangle. QML is not bad for a lot of things, but so far using the non-QML rectangular selection in either Kaption or KSnip feels so much faster and pleasant to use.

If you look at some of the commits, I think you'll see that Boudhayan came to the same conclusion eventually wrt. to his rewrite of that UI to QML.

Given that, I don't feel like spending too much time on this right now, in particular adding Kirigami support. There are more fundamental problems to solve first. The better help text could still be kept in any later implementation, of course.


One more thing: Are you sure Kirigami.InlineMessage is the right thing to have? Maybe whatever the on-screen notification you get on Wayland when starting Spectacle uses would be more appropriate?

src/QuickEditor/EditorRoot.qml
281

See summary:

Setting the default visibility of the help texts to false avoids showing them initially for a brief moment.

You should be able to test this by following the test plan before and after the patch.

rkflx updated this revision to Diff 33374.May 1 2018, 8:48 AM
  • Use Label instead of Text.
rkflx added a comment.May 1 2018, 8:57 AM
  • Optional, but desirable: use Kirigami.Heading instead of Label or Text: it's like Label, but has fancy logic to allow you to increase or decrease the size in a way that respects the user's chosen font size.

We could also simply do this:

font.pixelSize: Qt.application.font.pixelSize * 1.3;

(via https://stackoverflow.com/a/43248475)

guotao added a subscriber: guotao.May 1 2018, 9:33 AM

TBH, I'd be in favour of replacing the QML parts entirely given the performance penalty of the current implementation (or at least rewrite everything in a way this is not an issue anymore). With that I specifically mean startup performance (which despite the QML cache is still pretty bad) as well as input lag when adjusting the rectangle. QML is not bad for a lot of things, but so far using the non-QML rectangular selection in either Kaption or KSnip feels so much faster and pleasant to use.

Agreed. Shall we abandon this in favor of D12626: Port QML Rectangle cropper to QWidget + QPainter?

rkflx added a comment.May 1 2018, 1:05 PM

Agreed. Shall we abandon this in favor of D12626: Port QML Rectangle cropper to QWidget + QPainter?

I'd rather not. A bird in the hand might be worth two in the bush (or something like that).

rkflx updated this revision to Diff 33424.May 1 2018, 7:07 PM
rkflx edited the summary of this revision. (Show Details)
rkflx edited the test plan for this revision. (Show Details)
  • Larger font size for main text (changed my mind a bit there)

Any more changes I should make in the short-term?

ngraham accepted this revision.May 1 2018, 7:17 PM

TBH I kinda liked the rounded corners. But oh well.

This revision is now accepted and ready to land.May 1 2018, 7:17 PM
rkflx updated this revision to Diff 33432.May 1 2018, 7:45 PM
rkflx edited the summary of this revision. (Show Details)
rkflx edited the test plan for this revision. (Show Details)
  • Please Nate

You are right, looks better. Next time, don't make me iterate through so many revisions, though :D

Happy now?

ngraham accepted this revision.May 1 2018, 7:57 PM
  • Please Nate

    You are right, looks better. Next time, don't make me iterate through so many revisions, though :D

    Happy now?

...Says the king of making people iterate through so many revisions! ;-)

Yes, I'm happy now. Ship it!

rkflx added a comment.May 1 2018, 7:59 PM

...Says the king of making people iterate through so many revisions! ;-)

Not really ;) It's usually one wishlist of behavioural changes, and one wishlist of code changes. Only in case the author adds new problems (or even features) or fails to fix things properly, more iterations are needed.

This revision was automatically updated to reflect the committed changes.