- User Since
- May 6 2017, 7:59 PM (58 w, 3 d)
We are pretty close now, just some final polishing 👍
LGTM, just a comment to keep the commit history spotless ;)
@muhlenpfordt Thanks for testing, I think you found an important flaw.
Mon, Jun 18
Yeah, the right thing to do in the long term would be to step back and rethink how copying URLs or pixmaps should work from a user/workflow perspective. I tried to start with some peculiarities in my comment above, but obviously this needs more thought. It should be simple, not a myriad of buttons and options, yet support all existing workflows.
Nice, works really great now. Some minor inline comments, then we are good to go.
Sun, Jun 17
Sorry I was busy last week, but thanks for the patch anyway. Could not look at it in detail yet, but here are some high-level comments:
- Did Nate mention that currently there is work underway in D12626: Port QML Rectangle cropper to QWidget + QPainter? Not sure what's the best course of action though, i.e. whether to rebase on the port now, or finish the QML-based patch and port later.
- With regards to the choice of shortcuts, I think users prefer consistency with other apps. For example in Inkscape and LibreOffice:
- ⇧ increases the distance per step
- Ctrl changes the type of action performed
- I find it a bit annoying how slow the feature is by default. Would it make sense to swap the the behaviour, i.e. you'd have to press the modifier to move slower?
- There's only one hurdle left to make selecting a region possible from the keyboard only: You have to perform at least the initial click. Ideas?
- Moving and resizing beyond the border of the screen should not be possible.
- It's probably not necessary to describe every option in detail and overload the help text (doubling the number of options and tripling the covered screen area). How about this:
Arrow keys: Move selection Hold Ctrl to resize, fine-tune with Shift
Thanks, preferable over D13221 indeed.
Thanks for the patch and sorry for not noticing earlier – in general it's best to set Spectacle as a reviewer so Spectacle's members will have a chance to know about and review the patch before it lands without acceptance.
Fri, Jun 8
Thanks for the update. Don't have time to look at this right now, but here's one more tip (which should come in handy for your next patches, of which I secretly hope there are many ;)
Thu, Jun 7
Okay, great. Thanks for investigating! I now changed to what you proposed, so we have something for 5.13.1.
Wed, Jun 6
Anything left to be done here? I guess this should land on the Plasma/5.13 branch, since it's still broken there (I almost wrote a patch…).
From the git log:
Change selection rect from solid to dashed line
@faridb Thanks for the patch ;)
Nice, LGTM now. I'll wait a couple of days whether we get any more updates on the bug, and then commit on your behalf.
Tue, Jun 5
Mon, Jun 4
For me it seem Gwenview is not at fault, and until we fix the issue elsewhere, here we only can work around the problem.
Sun, Jun 3
I'll reply in the other Diff, as updating the code of an already landed Diff is always a bit confusing later on…
@kapillamba4 Apologies for the extremely long wait, I now looked at your patch (feel free to poke other people in the future if your original reviewer is too busy…).
@dporobic Thanks logging those issues! (Once the code lives in KDE's repos, we should probably use tasks on Phabricator for tracking…)
Thanks for the update ;)
Sat, Jun 2
I'll land this on the stable branch, i.e. Applications/18.04, as it is a nice bugfix for an annoying problem.
Excellent. I tested everything again, and the issue with the folders not appearing first anymore is now solved.
Ah, one more thing: You might want to update the summary a bit to reflect the current state of the patch, as it will become the commit message. Simply click on Edit Revision on the top.
I'll land it first thing in the morning!
Thanks, good point. I'm now using an enum. I wonder how I missed this obvious improvement.
Nice! And thanks for using Arcanist to submit the patch.
Great, LGTM now except for two minor comments.
Awesome, I did not expect that the fix for the shortcuts would be so simple. Thanks for your patience, we are nearly there…
Thanks for the update and sorry it took me a while to look at it.
Thanks, LGTM now :)
Fri, Jun 1
Thanks for the quick review!
@faridb Thanks for the review ;)
Thanks again for the patch, works great for folders with only images in them and even title/summary/test plan are looking excellent.
Allow to show persistent messages.
Thu, May 31
increase the timeout for the notification
I'd vote for disabling the timeout entirely in that case, because nothing is
more annoying than clicking a split second too late (which can happen
regardless of the timeout you choose).
+10. Heck, I'd support doing that now, because it's really annoying to miss
copying the URL because you were a tiny bit too late.
change the string to just "Copy link", ommitting the "to clipboard" part.