User Details
- User Since
- May 6 2017, 7:59 PM (358 w, 2 d)
- Availability
- Available
Aug 27 2018
Now that 18.08 is released, it's finally time for the code review part of your patch. As mentioned before, I don't have any hardware to test this. Mouse-based usage still seems to be fine, with one exception though (see below).
Aug 25 2018
Hi everyone. Sorry to say, but in light of recent events I lost all motivation to further contribute to Plasma. As for the Gwenview integration, Huon told me he's not very likely to contribute code anymore either. Not sure what this all means, but good luck anyway.
Sorry to say, but in light of recent events I lost all motivation to further contribute to Plasma. Someone else should take over.
Sorry to say, but in light of recent events elsewhere I lost all motivation to further contribute to Plasma (this has nothing to do with you or your patch). Please find another reviewer.
Aug 24 2018
Abandoning after reassessing priorities.
Aug 23 2018
@ngraham I have suggested this multiple times in response to similar apologies of yours before, and here I'm only doing it again because I still care at least a little bit and I still hope there will a chance for change: Please let go of your assumptions regarding hidden agendas or how often, where and why I comment on your work. Nobody is here to specifically pick on you, I'm sorry that you still seem to get that impression.
Oh well, the joys of diverging branches. Only the "close" vs "cancel" button types were accounted for, but not the (now) differing button boxes themselves. Patch in D15014. Glad you checked again!
Aug 22 2018
Aug 21 2018
I was not able to reproduce showing a wrong ratio in the checkbox, but CropRatioIndex=39816368 in gwenviewrc is certainly not correct ;)
@safaalfulaij Thanks for the links, those are quite interesting. So there are actually two different use cases for spinboxes:
- Changing a value only a couple of ticks (e.g. the number of copies when printing), where you need precise control and therefore are likely to click.
- Changing a value over a wider range (e.g. a width or height of an image), where scrolling is faster and does not need be precise. Those spinboxes are probably found in large professional apps with dense interfaces and less space for individual +/- buttons.
Aug 20 2018
Keyboard handling solved as requested and tests are running again, so now I can get back to what I was actually working on ;)
Hi, thanks for your patch. Sorry for not noticing it earlier, we don't get a notification if you don't set Gwenview as either the reviewer or project tag.
@ngraham Could you comment on why (on your personal machine) you went for increasing the font size instead of changing the DPI value? (If this is a UI issue, telemetry might give skewed results, BTW.)
- Fix crash
- Use int
Aug 19 2018
I tried your test plan, which works as advertised. Code LGTM too. Let's get rid of kdelibs4support…
Another idea I had (more suited for master, though): In addition to Enter and double-clicking I could also make Space accept a tool. Instead of doing nothing this could be handy for two-handed use (left hand on the keyboard, right hand on the mouse). Thoughts?
Aug 18 2018
(Please ignore the previous attempt ;)
Sorry for the wait, but I did not get to the focus/hover issue yet (and thus did not start with the actual review).
Ah, right. So doing the right thing here actually uncovers hidden problems elsewhere. Since you already solved them in
D14485, I'll depend on your Diff here.
Wow, you fixed it already! This also solves the problem of not being able to make the window smaller after disabling Advanced settings again.
Aug 17 2018
Aug 16 2018
Disable shadow for marker tool
Yay!
Sounds simple once you know the trick, but to get there was a bit long-winded…