D13450: Add arrow keys to move and resize selection rectangle
Henrik Fehlauer
noreply at phabricator.kde.org
Wed Jul 18 00:53:44 BST 2018
rkflx requested changes to this revision.
rkflx added a comment.
This revision now requires changes to proceed.
@sharvey Thanks for the updates. As always, first I'd like to iterate to get the patch working properly, and then I'll look at the code in detail. As I'm now put under pressure again to squeeze the patch into 18.08, we'll see how it goes (in general rushing leads to bugs, see last Plasma release…). First of all:
I still disagree regarding the default speed selection. We determined by looking at other apps that [⇧] is the modifier to use, and I argued (in line with what I meant when triaging the bug) that for making the rectangle feature useful for keyboard users by default the movement should be fast. BTW, this is also what KWin is doing, and I see no reason at all why Spectacle should deviate from that standard. (See above for even more arguments.) I don't feel comfortable approving the current default, sorry.
Apart from that, the patch now works well for the most part, good job (and thanks to @abalaji for commenting on some things). In addition to some inline comments below, I found those issues:
- When the rectangle covers the whole screen, I'm unable to resize, i.e. make the rectangle smaller again.
- I'm unable to fine-tune both size and position with HiDPI scaling active (see also inline comment for the likely cause).
- Selecting with the mouse sets a minimum size, however resizing with the arrow keys allows to resize to 0x0px.
---
To summarize, here's what I propose: If we can reach agreement on the default and you are fast with the string changes still required, the patch can go in for the Beta. Then you have time to work on the other points until the RC (at which point we have to decide whether to ship or to revert again, based on your progress).
---
In D13450#283620 <https://phabricator.kde.org/D13450#283620>, @abalaji wrote:
> A few thing tho @sharvey, it looks like currently the resize functionality only moves the bottom right corner, and I've preserved that. But was wondering if we can add in Ctrl or something to control that. Maybe something like:
>
> When Alt is pressed down:
>
> - We are in "expand" mode by default. Pressing left expands selection on the left by moving the left border outwards to the left, etc.
> - Also holding Ctrl enables a "shrink" mode. Now pressing left shrinks the selection from the right by moving the right border inwards, as if we are nudging it from the right towards the left, etc.
I think that's too complicated, and you still need two steps to get to the final selection (just like setting one corner by moving and the other corner by resizing are two steps too).
INLINE COMMENTS
> index.docbook:154
> +
> + <para>You can use the arrow keys to move and adjust the rectangle. Pressing the arrow keys will slowly move the rectangle. Holding the <keycap>Shift</keycap> while using the arrow keys will move the rectangle more quickly. Holding the <keycap>Alt</keycap> key while using the arrows will adjust the size of the rectangle.</para>
> +
"Holding the Shift while": Either remove "the", or add "key".
"using the arrows": Change to "Using the arrow keys"?
> EditorRoot.qml:41
> property int magOffset: 32;
> + property int largeChange: 15 / Screen.devicePixelRatio;
> + property int smallChange: 1 / Screen.devicePixelRatio
That does not seem right, because this will result in slower movement on a HiDPI screen compared to a regular screen of the same size. (It's needed for the 1px case, of course.)
> EditorRoot.qml:42
> + property int largeChange: 15 / Screen.devicePixelRatio;
> + property int smallChange: 1 / Screen.devicePixelRatio
>
That looks suspicious: How will you be able to store the result accurately in an `int`?
> EditorRoot.qml:54-57
> + "x": xx,
> + "y": yy,
> + "height": hh,
> + "width": ww
Unrelated whitespace change, please revert.
> EditorRoot.qml:84
> +
> + var delta; // valid for either direction
> +
Without the context of the patch in mind, this is a bit hard to understand what it is about when you stumble upon this while reading the code. Could you come up with a better name and/or comment?
> EditorRoot.qml:92
> + // nested switches for arrow keys based on modifier keys
> + switch(event.modifiers) {
> +
Insert space after `switch`, please. Repeat for all other occurrences.
> EditorRoot.qml:98
> + delta = checkBounds(-smallChange, 0, "left");
> + selection.x = selection.x + delta;
> + break;
Do you know about the `+=` operator, which would be more concise?
> EditorRoot.qml:116-137
> + case Qt.ShiftModifier:
> + switch(event.key) {
> + case Qt.Key_Left:
> + delta = checkBounds(-largeChange, 0, "left");
> + selection.x = selection.x + delta;
> + break;
> +
This duplicates most of the code for `Qt.NoModifier`. Could you instead assign `largeChange` or `smallChange` to a variable based on the modifier, which you then pass to `checkBounds`?
Also this is likely independent from whether you are moving or resizing (where all of this is duplicated again currently).
> EditorRoot.qml:173
> +
> + case(Qt.ShiftModifier + Qt.AltModifier):
> + switch(event.key) {
Insert space after `case`.
> EditorRoot.qml:210
> +
> + function checkBounds(deltaX, deltaY, direction) {
> +
As the mapping between `Qt.Key_*` and `"*"` is static, for `direction` you could pass the key directly instead of creating an extra string.
> EditorRoot.qml:257
> + }
> +
> + case "down":
Missing `break` (not that it would matter with those `returns`, but there should be at least a consistent style).
> EditorRoot.qml:371
> }
> -
> }
Unrelated whitespace change.
> EditorRoot.qml:443-507
> + height: midHelpTextElement.height + 20;
> + width: midHelpTextElement.width + 20;
> radius: 4;
> border.color: systemPalette.windowText;
> color: labelBackgroundColour;
>
> visible: false;
Currently I don't understand what all those changes are for and how they relate to the topic of the patch (and I would disagree with some).
Contrary to @ngraham I'd say this actually is a visual regression.
> EditorRoot.qml:512
> + Label {
> + text: i18n("Arrow Keys:");
> + Layout.alignment: Qt.AlignLeft && Qt.AlignTop;
Would it make sense to use the same capitalization as above, i.e. "keys" instead of "Keys"?
> EditorRoot.qml:513
> + text: i18n("Arrow Keys:");
> + Layout.alignment: Qt.AlignLeft && Qt.AlignTop;
> + }
Do you mean `|` instead of `&&` by any chance?
> EditorRoot.qml:516
> + Label { text: i18n("Move selection rectangle \n" +
> + "Alt to resize, Shift for %1px change", largeChange); }
> }
I would suggest to keep "Esc" at the bottom, to make it easier to find. "Arrow keys" could go after "Shift", so "Right-click" would still be next to "Esc" to which it belongs.
> EditorRoot.qml:516
> + Label { text: i18n("Move selection rectangle \n" +
> + "Alt to resize, Shift for %1px change", largeChange); }
> }
"15px change" does sound a bit strange: What is "change", and why do "15px" matter? Let me repeat my suggestion from above:
Hold Alt to resize, fine-tune with Shift
REPOSITORY
R166 Spectacle
REVISION DETAIL
https://phabricator.kde.org/D13450
To: sharvey, rkflx, ngraham, #spectacle, yurchor
Cc: ltoscano, kde-doc-english, abalaji, #spectacle, skadinna
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-doc-english/attachments/20180717/01fdc706/attachment.html>
More information about the kde-doc-english
mailing list