| ngraham | |
| rkflx |
| Gwenview |
Similar to zooming using Ctrl+touchpad two-finger scroll, using touchpad scrolling
in image view when mouse wheel behaviour is set to Browse, browsing images is way
too fast.
This implements the same fix in D7744: Reduce hyper-sensitive touchpad scroll-zoom speed for this case.
BUG: 388353
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
Wonderful. Works just as well as the other patch, and the mouse use case is still fine too.
My impulse is to land this, as it is a small change based on another we know works, and that has been tested using multiple input devices. @rkflx, if I don't hear anything in the next 48 hours, I'll land it.
Okay, I'll look at it in the evening. I don't really like pushing this in front of other reviews which have been waiting for much longer, but oh well…
Also note that @huoni has commit access now.
Thanks Henrik. This is a pretty darn simple change, and it can be tested adequately in 5-10 minutes.
Sure, but it's also a hack and I want to understand the real issue before creating even more maintenance problems (note I was not around when the other patch got in).
Looked at it now: Great patch, excellent commit message. Makes the review easy ;) As mentioned elsewhere, I cannot test this with a touchpad, but I'll trust you both that it works as advertised with any type of driver.
This can go in for now, I only have a small comment:
- Enable thumbnail bar
To make this less confusing (as that's not where you should scroll), perhaps add "to check how many images are scrolled at once".
@huoni Good luck with landing on your own ;) (Let me know if you have any questions with that.)
Still I wonder whether D11200 and D7744 should be reverted at some point in the future.
Reading the patch and the bug, it seems to me that said Qt patch is about scrolling too fast and not too slow like Nate claims. Ideally we would test the Qt patch beforehand… I assume the slow-scrolling problems in other areas are just caused by overzealous mitigations in Plasma or Kirigami, which broke it for users where it used to work before. Remember what Marco said:
Apart from the maintainability aspect, at least for the zooming case it would probably make more sense to implement smooth zooming instead of simply calling the regular coarse-grained zoom action.
In addition, both patches are kinda ugly in the sense that when starting to scroll after moving the mouse, controlWheelAccumulatedDelta is not reset and still has values from earlier on.
Anyway, that's probably for later. Thoughts?
@huoni Good luck with landing on your own ;) (Let me know if you have any questions with that.)
Thanks. One question semi-related to landing - if I understand the release cycle correctly, 17.XX is finished, so currently there is no "stable" branch to commit to. 18.04 will be the next major release based on master, and after that, 18.XX will be the "stable" branch. Is all that correct?
Thought of another - regarding the FIXED-IN keyword. Is there an easy way to determine this? For this patch, would it be 18.04?
Exactly! So go ahead and land it.
Thought of another - regarding the FIXED-IN keyword. Is there an easy way to determine this? For this patch, would it be 18.04?
Yup! And here's the documentation: https://community.kde.org/Guidelines_and_HOWTOs/Write_a_version_string
Thanks. Another thing I'm not sure about. Does arc land let you edit the commit message? E.g. to add keywords like 'FIXED-IN' and 'Differential Revision'? Or do I need to edit the commit locally, then call arc land?
arc land doesn't give you a chance to do that; you need to do it beforehand. I generally use the web interface (and then re-download the patch), but I believe you can use arc diff --amend --edit --verbatim as well.
Right okay, why then do none of my previous patches have the FIXED-IN keyword? Is that just because they were landed on my behalf, and it's optional? It just seems odd to add that keyword to a diff so I want to make sure I do things right.
Yep. All that keyword does is populate the "Version Fixed in" field in the bug. If you don't add the keyword, you can always fill it in yourself.
Some more onboarding remarks below. I think they are important, so sorry for not working on other reviews first ;)
arc land will set 'Differential Revision' for you, no need to do this manually.
arc land doesn't give you a chance to do that; you need to do it beforehand. I generally use the web interface (and then re-download the patch),
You don't need to re-download the patch for changes to the commit message, arc land will call arc amend for you which updates the commit message. Calling arc patch again is only necessary if the author changed code of the patch itself.
but I believe you can use arc diff --amend --edit --verbatim as well.
That's also not quite correct, which I find surprising given Nate is the primary author of https://community.kde.org/Infrastructure/Phabricator where this is all explained in detail. If you want to change the commit message locally before landing, do this:
There is another way involving git push, but it lacks all of the sanity checks built into Arcanist, so I'd recommend against it. Also, in general you should avoid changing the commit message too much after your reviewer has accepted the Diff (except for typos, and other small changes like the FIXED-IN tag).
Yeah, I find it odd too nowadays. The reason it was missing is that I did not spend the effort to add it (kinda set a bad example, sorry for that). I know Nate considers it important, but in reality the keyword is a relic from the past: This was used by some tooling in the old days (think CVS and SVN) to generate reports for releases, but if you go to https://www.kde.org/announcements/ today, you'll see that is just uses Git tags to determine what was committed when. It still has some usefulness today, but only little:
IOW, add the keyword to be a nice citizen and help Nate, but if you forget it nothing will break really.
18.04 will be the next major release based on master, and after that, 18.XX will be the "stable" branch. Is all that correct?
Nearly correct, because you have to make a distinction between branches and tags in Git terms:
No worries :) – I usually test out new documentation on Phab, and then the good parts end up on he wiki eventually, often thanks to Nate.
So my preference here wold be to make it the default setting in "advanced" mode, but not in basic mode. The reason being that people not using advanced mode might not want to constrain the proportions to those of the current image, and shouldn't have to figure out that they need to open the Advanced settings to do it.
Perhaps we could put a checkbox to control this in the Basic view (since it's bound to be a very frequently used/toggled setting), and then put it in the Ratio menu in the Advanced view, the
Speaking of that, I just added a section: https://community.kde.org/Infrastructure/Phabricator#How_to_review_someone_else.27s_patch
Feel free to tweak as necessary, and I expect we'll evolve it over time just as we have the other parts of that page.
Speaking of that, I just added a section: https://community.kde.org/Infrastructure/Phabricator#How_to_review_someone_else.27s_patch
Cool idea, but I don't have time to improve this ATM. It's currently only describing QA, the whole code review part is missing, which is at least as important! Please add a note regarding that.
That's the part I'm least confident on myself, being at best an amateur programmer. Hopefully someone else (preferably a core developer) will come along and add some information about code review.
Don't expect anyone else to do the work for you. At least you could add a note that there is something missing.
I'm not, but it is a wiki. Nevertheless, that's a good idea. I'll add a note.
I hope I haven't upset you, Henrik. I know that our discussion about single-click vs double-click got pretty intense, but I want to let you know that I have tremendous respect your skills, perspectives, and thoroughness. It feels weird to find myself disagreeing with you, and I want to make sure it doesn't damage our relationship.
@ngraham I'm fine. But please understand that putting something incomplete in a wiki and adding a comment here does not give me 2 extra hours in the day to improve it for you, even though I'd love to. I tend to work in-depth, you sometimes only touch the surface.
if I don't hear anything in the next 48 hours, I'll land it.
This is a pretty darn simple change, and it can be tested adequately in 5-10 minutes.
Please respect that others sometimes need more time, because they have another style of working. Or are you attempting to DoS me?
No, I'm not trying to do anything to you! I'm sorry for having given you that impression. And I'm also sorry about rushing you; I know you've asked for more time before, and I'll try to get better about not rushing you and respecting your style.
To explain my style a bit more, I tend to follow the philosophy that something is better than nothing; action is better than stagnancy; the good and the now are better than the perfect and the later. An incomplete section on a wiki page is a great example: the content that is there starts helping people immediately, and the missing content invites improvement from others and serves as a reminder for the original author to finish it. Whereas waiting for the time to do something perfectly often results in it never getting done at all, or getting done so slowly or so late that all the energy in the system has drained away and the work becomes a slog.
Neither of these is really right or wrong, IMHO. There's a time and a place for each: Too much rushing around without consideration leads to buggy, half-baked work, and a neverending cycle of fixing unnecessary regressions; too much deliberation and discussion consumes most of the energy before the work even gets started and makes an organization feel ponderous and unappealing to outsiders. Might also be an American vs European style thing, I dunno.
Ideally we all figure out how to move into our natural roles and positions based on our personal styles. I'll try to stay out of your way a bit more, if that's what you need. I know my hare style can be really stressful to more tortoise-style people (at least this is what my wife tells me!) and I'll try to be more conscious of that in the future.