flameshot-org / flameshot

Powerful yet simple to use screenshot software :desktop_computer: :camera_flash:
https://flameshot.org
GNU General Public License v3.0
24.02k stars 1.53k forks source link

General improvements to pinned image scaling behaviour #3428

Open schwubdiwub opened 7 months ago

schwubdiwub commented 7 months ago

I changed a few things, all involving the scaling behaviour of pinned images.

Foremost, I changed the formula for calculating the change of scale. Now the rate of change scales with the current image size. Before, for example, an image with original size of 100x100 would change size at another speed than an image with original size of 400x400 that had been downsized to 100x100. Originally bigger images changed their size faster than originally smaller images. Another thing about scaling speed I changed is, that it is now possible to hold ctrl / shift while scrolling to increase / decrease the scaling speed by 5 times. Since this involves hotkeys, this might be something to discuss?

Then, when creating a screenshot which size was smaller than the hard-coded minimum image size of 100, scaling would cause the pinned image to make a jump to hard-coded min size dimensions and prevented to scale in the range of original size and hard-coded min size. Now the lower bound of scaling is set to whatever is smaller, the original size or the hard-coded min size.

Lastly, I fixed the behaviour when scaling down to minimum image size and then keeping scrolling down. This caused the internal scaling factor for the image to further decrease, which in turn caused that trying to scale up would not react immediately, but you had to keep scrolling up until the internal scaling factor reached its value equivalent to min image size and after that point the image size started to increase again. Now the internal scaling value is limited as well when the image size is limited by the min image size.

If there is anything wrong about my way of contributing, please let me know, since this and another pull request are my first contributions to this project. Feedback is appreciated 😄

mmahmoudian commented 7 months ago

Thanks for the contribution👍. We generally prefer to discuss such big changes first before implementing them. This procedure would have clarified some of the questions you yourself also posed.

Regardless, here are my comments (we should wait for other devs' comments too):

Originally bigger images changed their size faster than originally smaller images.

Yes, this was something that I also observed, and was also a bit hurting my workflow as well.

Then, when creating a screenshot which size was smaller than the hard-coded minimum image size of 100, scaling would cause the pinned image to make a jump to hard-coded min size dimensions and prevented to scale in the range of original size and hard-coded min size. Now the lower bound of scaling is set to whatever is smaller, the original size or the hard-coded min size.

This is nice, but it creates a problem: losing the pinned window. Considering that the pinned image windows does not have decorations, if the size is small, it would be very easy to lose it. I'm not against your change, but I think we should have a mechanism to help user find/locate the window if it is very small. For instance we can set the key binding like = to always change the size back to the original size unless the original size is smaller than 100 pixels which then we actually increase the size to 100 pixels.

Now the internal scaling value is limited as well when the image size is limited by the min image size.

I like this change too. I have also observed this behavior in my test as well, but it was so rare that I thought it is not hurting the user's experience.

mmahmoudian commented 7 months ago

There's also one point that you have used hard-coded key bindings but there is no UI representation of them. I would suggest adding them to the right click menu as well so that the user can learn/discover the key bindings from that menu.

schwubdiwub commented 7 months ago

Yeah, I read the contribution guidelines, but I was not sure where to draw the line between incremental improvements and bigger changes. And as I was already fiddling with the code, I took this route to find out more about the process. I can create an issue for the proposed changes. Or we can keep the discussion here, since we are already discussing?

This is nice, but it creates a problem: losing the pinned window. Considering that the pinned image windows does not have decorations, if the size is small, it would be very easy to lose it. I'm not against your change, but I think we should have a mechanism to help user find/locate the window if it is very small. For instance we can set the key binding like = to always change the size back to the original size unless the original size is smaller than 100 pixels which then we actually increase the size to 100 pixels.

That is a good point. And I like the key binding idea, but then there is the problem of keyboard focus. If the widget is already tiny, that the user can't locate it, how can the user control that the use of the key binding will affect the lost widget? And a global key binding that affects all widgets is a no-go in my opinion, since this will destroy an already assembled collection of pinned images and will possibly create clutter on the screen. I have seen, in #2984, that there is a desire to make pinned images minimizable again. This could help user in case of lost widgets as well. Another idea is to create a separate "pinned images managing" dialog, to handle this case and possibly add other features, too. But at the same time, I think this will leave the intended scope of this project 😆

There's also one point that you have used hard-coded key bindings but there is no UI representation of them. I would suggest adding them to the right click menu as well so that the user can learn/discover the key bindings from that menu.

That is a good point, too. Right now, the implementation does not align with the behaviour of checked actions found in such menus. So it would be possible to add an action or two that do nothing when clicked, but are hinting the user at the key bindings. But at the same time, it could be confusing for the user to have actions that actually do nothing. Maybe add a tool tip to the widget, which serves this purpose? Changing the behaviour of the hotkeys to align with the behaviour of checked action would be possible, too. But I think this will decrease the usability for this feature.

mmahmoudian commented 7 months ago

I can create an issue for the proposed changes. Or we can keep the discussion here, since we are already discussing?

Let's discuss here 😊

If the widget is already tiny, that the user can't locate it, how can the user control that the use of the key binding will affect the lost widget?

Yes, you are correct. We should find a solution for this issue. I like the idea of a panel for pins, but that might be overkill. Maybe we can have all the pinned images to become 100x100 and get arranged in a grid? Although I have a feeling that it is a bad idea because of Wayland and tiling window managers. Making them minimizable is good since Atl-tab can let the user to cycle through windows as well.

it could be confusing for the user to have actions that actually do nothing. Maybe add a tool tip to the widget, which serves this purpose?

Perhaps we can have a window show keybindings when ? is pressed. Like in a separate window (normal window with decorations and etc.)

schwubdiwub commented 7 months ago

Maybe we can have all the pinned images to become 100x100 and get arranged in a grid? Although I have a feeling that it is a bad idea because of Wayland and tiling window managers.

And, as well, in case someone has a few pinned images arranged in a certain layout to fit over another window, the auto arrangement into a grid would destroy the user's layout. So making them minimizable is the better solution in my opinion. And it handles an open issue at the same time.

Perhaps we can have a window show keybindings when ? is pressed. Like in a separate window (normal window with decorations and etc.)

I really like this idea, since there are other hidden hotkeys for changing the opacity that are not visible to the user (at least I didn't see where). We can put all those key bindings in the dialog and maybe add a few more for the other right click options, like arrow keys to trigger left and right rotation.