Zoom for the editor (Feature-84) #201
Conversation
syncing with upsteam
|
Hey, thanks for the PR 👍 I'll do some testing. I agree that we should re-assign Ctrl + + and -, the other feature is not widely known anyway. Personally I'd prefer Ctrl + 0 for 100%, I think that's what most popular browsers do and should known for most users. |
|
Yeah, I've got another response about Ctrl+0 already. On the other hand, 0 ) is closer to other keys on main part of keyboard and might be more natural for users with short keyboards without numerical part... |
|
Thanks again, so I did some testing, the implementation works great in most situations - good job 🙇 I have just found some smaller issues, and a few points open for discussion (this includes the shortcuts already mentioned above). Adding these as tasks here (we could also consider to make separate tickets for these, in case any of them takes longer or needs a lot of discussion): Issues:
Open for discussion:
|
|
@KillyMXI Great PR! Ctrl + 0 would make sense, but I agree that digit shortcuts sould be reserved for other features: line thickness, colors, ... What about the spacebar? Nowhere used so far and I do not see a use case |
|
@Lakritzator would you like to have a look at the code? You're the one with the deepest insight into the current codebase. |
I have to figure out something to test this. I was hoping I handled this well. But this sounds like .Net might not return what I expect it to.
I also found an omission on my side:
How I missed this... I'll look into it.
I would prefer it to take least amount of screen space. The main goal of Greenshot is capturing what's going on in other apps, after all.
And this is a good point. I'll look into it.
Latter is kinda pointless for me. As I mentioned above, I don't see a point of Greenshot editor taking more space than it needs to show the screenshot.
I'm not a fan of it. It annoys me more often than it helps, when I accidentally trigger zoom with scroll in any apps. And the way different mice emit scroll events - is a pain on it's own.
Yeah, I'm kinda afraid of what might be happening on different layouts. Good point for Ctrl+0.
Agree, it looks best from the proximity on the main keyboard. Although it hurts me from how otherwise senseless it is.
Yeah, I had that in mind. |
|
I just checked out the PR, and am now working with it. It gives me a good vibe, and I'm very impressed so far!!! Most of the issues have been stated by Jens already. About the zoom with the ctrl + mousewheel, I myself am a big fan of this! I will now take a look at the code. |
|
Ah the first issue I found, is that the range of the speech bubble tail is limited when the zoom < 100% |
|
When I am at < 100% the crop doesn't crop the correct (selected) area. |
|
Yeah. jklingen pointed at this already. My bad, will fix soon. |
|
Sorry, I somehow missed that... :) |
This is mysterious. I think I had it at some moment, but I can't reproduce it since. |
|
I like best fit, but it doesn't work after I rotate the capture. |
|
Hmm. Can't reproduce again. Might be same cause with that crop issue? Upd: found a way to consistently reproduce the crop issue, but not the "Best Fit after rotate" issue. |
|
I can have a deeper look at the first two issues I reported, the ones you cannot reproduce. Probably tomorrow. |
|
About the best fit issue I mention before, I think I understand what is the issue. I captured a screenshot with 650x400 px, if I use best fit, it will enlarge to 200% What is the issue? I was expecting that the zoom can use arbitrary sizes, but it will probably try find one of the predefines sizes which has the best fit. And as 200% doesn't fit, it sticks with 100%. You could say, it works as designed. But I'm not sure if this makes sense... |
|
I finally managed to do a review. First this: I'm amazed at what you did! You managed to understand the not quite simple Greenshot editor, and made a not trivial change to it. And on top of this, you managed to do this in such a way that it even looks like how we would do it. Chapeau! Well done!!! I was only able to do minor nit-picking, all my remarks have little value... |
Anything between full hundreds (e.g. 150% or 133%) can't be shown "pixel-perfect" - it looks really distorted. I just checked what Paint.NET does. It enables smoothing for 150%. |
|
You brought up a good point (therefore the 👍 ) Right now you convinced me that it doesn't make sense, but I will talk about it with the guys and I have the right to change my mind. 😄 |
|
Just a side note... it will not be an easy merge into develop 😢 We should also look at adding tabs to the editor 😁 |
|
In fact, I started this in the develop branch and switched to release/1.3 half way through 😅 |
|
That was the right choice, 2.0 is far from finished. Maybe partly due to some of the dotnet core 3.x short comings but mainly because there are still a lot of bugs and things to do. |
- better interface for coordinate translation; - correct context menu location; - speech bubble tail can be dragged over the whole image.
Even at 100% GDI+ manages to do some smoothing. Also minor optimization - not messing with Graphics state when not needed.
|
I think I'm done. |
|
🎉 I'll check out the funny jitter, which definitely was already there before. Today was a German holiday and we planned to renovate a room for my kid, which I started in the evenings of this week and will finish tomorrow. I'll check the code as soon as I have some time behind my pc. I believe it's awesome!! |
|
Wow, you're quick 😲 thanks for the update. I gave it another test, and from my perspective it looks great. Thanks for fixing the very-tall-screenshot issue; and I can no longer see jumping pixels 🎉 Also:
great idea - that's an awesome addition to the feature 👍 |
|
I am eager for a test release :-) It is not clearly visible from the initial screenshot, but shouldn't the four most important zoom features - which have an own icon - in, out, best fit and actual size be available on the tool menu to? as an own section after the redo. thanks to all for the efforts! |
|
I would argue against adding zoom icons to the top toolbar at this time.
I thought about adding extra icons to the status bar, but I decided that current design is very straightforward and sufficient for practical use. |
|
Just to avoid any time waste due to miscommunication, I would love to know: What exactly is "In progress" now? I'm not aware of any blocking issues or features left. So in case you expect something - be sure to state it. |
|
Sorry, I was really dead tired yesterday... having a 4yo is not without penalty in this time. I wanted to start my work at 07:00, due to him it was 09:45, so now at ~22 hours I finally finished work and I can review your changes... long days. About your final question: I just added this issue to the things I want to have in 1.3 and while I didn't finish the review yet, it is automatically on "in progress". I just finished reviewing all the code, amazing job! During my test I found more weird drawing artifacts, I am sure that they were already there before this PR, at least 99%. Just amazed about how tough it is to test such things. I do wonder where the Fraction struct comes from, not that we receive code which is under a conflicting license. Without having this cleared I'm afraid I cannot merge yet, do you remember? Due to that question, and the size of the PR, we are finally activating the CLA bot, to protect us both. Not that you wonder, we had this on our roadmap for quite some time now. |
|
Fraction struct is completely mine. I also briefly thought about making a more fleshed out analogue as a Nuget package, but I will probably find no motivation for that unless I run into another use for it. |
|
Any time estimates? |
|
@KillyMXI we have finally set up the CLA Assistent today, but we want to give it another test. If everything is fine, from my point of view we're ready to merge. |
|
I tested the CLA-Assistant in another PR I am somehow trying to make the assistant check this PR... if you wonder where all the SPAM came from 🤷 |
|
|
|
Signed |
|
(This was started before your signed, I was busy putting my kid to bed, he still doesn't sleep and it'S 23:00 hours) @KillyMXI so we got it working, your PR is really cool but we found we need to protect you and us from contributions which cause discussions. You only need to confirm the CLA once, no matter how many PRs you made, it's just for our cooperation. Why we stated with this now? Nobody made us a PR as big as you did!!! |
"KillyMXI", linked to my GitHub profile, should be good enough. |
|
@KillyMXI your PR is awesome. I really missed the Zoom feature, thank you so much.
Well, let me put my 2 cents: IMHO CTRL+Wheel is really natural (and widespread), and all the softwares I use day-by-day have it: Firefox, Chrome, paint.NET, GIMP, Kdenlive, Shotcut, Notepad++, Office, LibreOffice, VisualStudio, VSCode to name a few. I respect your opinion and work, but please reconsider adding it, maybe with an option to enable/disable it. |
|
I suggest to open a separate issue to track mouse scroll zoom. |
|
Thank you @KillyMXI - great work! Scrolling on the windows up and down as well left and right - nice implemenation with the shift + scroll - isn't so handy. |
|
appologize, dragging the canvas in Acrobat Reader is SPACE + mouse (not shift) |
Zoom implementation
Notes:
Shortcuts Ctrl+
+,-were used for another feature. Considering these shortcuts are wastly more common for the Zoom feature, I suggest to move the previously existing feature to Ctrl+ Shift ++,-Shortcuts for Actual size (100%) and Best Fit (Ctrl+/and Ctrl+*) are chosen for the proximity on the keyboard and because of possible collisions for other choices. Of the software I use, Ctrl+/for actual size is also used in XnView MP."Zoom" menu instead of "View" menu - because there are no other View features so far, I think it's more accessible this way.
Zoom display on the bottom right shows the same menu on click. Menu lists all possible values for quick access. I think this UI solution makes a good balance between usability and code complexity.
Update:
Summary of behavior changes: