Rewritten rectangle/arrow key movement & resizing functionality.
Eliminates problem where rectangle edges were resized to touch a screen edge but could not be sized back.
Also halts resizing at 20x20px - matches minimum size when mouse is clicked to begin selection.
Details
Details
- Reviewers
rkflx ngraham - Group Reviewers
Spectacle - Commits
- R166:f484ee979bed: Rework rectangle/arrow key handling to avoid "stuck edges" and 0x0px rectangle
- Download patch & apply to Spectacle
- Begin a rectangular selection by dragging a rectangle with the mouse
- Use Arrows keys to move rectangle to various screen edges
- Use Alt + Arrow to ensure rectangle edge releases from screen edge
- Use Alt + Arrow and attempt to make the smallest rectangle possible It shold stop at 20x20px and not get any smaller
Diff Detail
Diff Detail
- Repository
- R166 Spectacle
- Branch
- fix-stuck-edges (branched from master)
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 1143 Build 1156: arc lint + arc unit
| src/QuickEditor/EditorRoot.qml | ||
|---|---|---|
| 216 | The use of action = with the ternary operator is to quiet QtCreator. According to the Javascript/ECMAScript standards, a ternary operator can be used without a variable assignment, but QtCreator clutters up the editor with warnings. action never gets used, except as a placeholder. | |
Comment Actions
Here you go - a freshly rewritten arrow/rectangle handler. It's still got a fair bit of repetition, which I'd like to work on at some point. This version does not get "stuck" at the screen edges. It also stops resizing the rectangle to a minimum of 20x20px, the same as the initial mouse click to start the rectangle.
There's a tiny amount of whitespace movement, trying to get all the brackets and code blocks aligned with each other. It's a whole lot of indentation; I wanted to keep it consistent. That's from my time writing Python code.
Comment Actions
Thanks, this fixes the issue of getting stuck, as well as the minimum size.
However, there is a new problem: Alt+↑ as well as Alt+⇧+↑ resize twice the amount compared to their counterparts.
I also added some comments regarding style, I consider their implementation optional.
As a more general tip, it should be easier to branch off of the stable branch (Applications/18.08) if you are submitting a bugfix, like so:
git checkout Applications/18.08 git pull arc feature fix-stuck-edges
Currently your patch is branched from master, which makes landing to stable more difficult, as it requires extra steps for rebasing and specifying the target branch.
| src/QuickEditor/EditorRoot.qml | ||
|---|---|---|
| 43 | Good code design avoids as much global state as possible. This variable is only really relevant for the bounds checking, though. See also the other inline comment. | |
| 198 | A switch for a bool looks really weird. Normally for bools an if is used, but what's better here is probably to make the action a (string) parameter of the function and keep the switch. Even better would be to write two functions (one for each action, i.e. move or resize). | |
| 203–204 | That's a bit difficult to read. Would the following simpler code also do what you want? selection.x += changeX;
if (selection.x < 0.0) {
selection.x = 0.0;
} | |
| 216 | Hmh, in general adding workarounds to the code to avoid warnings in an editor is an anti-pattern. Qt Creator might not be perfect, but often enough the warnings have a point. There would be no warning if you wrote that line like this: selection.y = newBottom <= screenMaxY ? selection.y + changeY
: screenMaxY - selection.height; | |
| 242–247 | Aren't you adding changeY two times to selection.height here? | |
| 252 | Note that selection.height + changeY will be assigned to action, which is not used later on, making calculating the sum a bit pointless in the first place. | |
| src/QuickEditor/EditorRoot.qml | ||
|---|---|---|
| 43 | I'm going to beg off this error as "knowing the logic, but not the style." I'll rework the patch without it. Thanks for another entry in my C++ style notebook. | |
| 198 | My goal in writing one function was to avoid duplication. A lot of the code will be the same (checking edges, etc). I will rework the function to use resize as a string. | |
| 216 | I confess I never felt right about the Qt Creator kludge. Thanks for the guidance. I will try to avoid "anti-patterns" going forward. | |
Comment Actions
@rkflx - I fixed the double-size movement for Alt + ↑ (and Alt + ⇧ + ↑ as well).
As you wrote, the others are poor style. I would prefer to address them in a separate patch, so that you may submit this as a "bugfix" prior to the 18.08 release.
I've rewritten the key and edge handling a half-dozen times thus far; I don't want to end up sweating a bugfix deadline while rewriting it again. Naturally, I'll do my level best to get it done before the 18.08 deadline, but I'd like to have the "stuck edges" and "0x0px" bugs fixed and filed. I'll work on my style separately.
Fair enough?
Comment Actions
a more general tip, it should be easier to branch off of the stable branch (Applications/18.08) if you are submitting a bugfix, like so:
git checkout Applications/18.08 git pull arc feature fix-stuck-edgesCurrently your patch is branched from master, which makes landing to stable more difficult, as it requires extra steps for rebasing and specifying the target branch.
I followed your directions to the letter, but this page still reads "branched from master".
I think "arc" is shorthand for "arcane" - hidden knowledge which take dedicated study for acolytes to master.
Comment Actions
Thanks, can confirm that the issue with doubled resizing is fixed. Accepting for now, even though the code is not the most concise yet.
Please commit to the stable branch.
I will now focus on getting D12626: Port QML Rectangle cropper to QWidget + QPainter in shape.
Sorry, my explanation could have been more clear: My example was for when you create a new bugfix. If you already created a branch named x, arc feature x will simply switch to that branch, but not create a new branch or rebase it. Arcanist will also tell you what it does in it's output if you pay attention to it, which will be different for both cases.
In your case, please follow https://community.kde.org/Infrastructure/Phabricator#Landing_on_the_.22Stable_branch.22.
Comment Actions
Thank you. This is my first "new feature" contribution. All the others have been bugfixes or tweaks. You should also know that I have a friend who's stuck on Windows and is very jealous of this feature.
I'll get it landed properly.