Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add button to fit image into current view #281

Merged
merged 4 commits into from Feb 15, 2022

Conversation

precla
Copy link
Contributor

@precla precla commented Feb 3, 2022

Adds a button so the user can fit the image to the current view.
Does not automatically fit image. Do we want that?

fitimage

as requested in: #21

@precla
Copy link
Contributor Author

precla commented Feb 3, 2022

The approach with -1 as the value is probably not the most straight forward and sane one. The goal was to add/change as few lines of code as possible...
Thoughts?

EDIT: Zoom value is not changing in the scrollbox. Have to fix that.. Done.

@precla precla force-pushed the fit_image_to_view branch 2 times, most recently from 42d0412 to affee00 Compare February 3, 2022 20:47
Copy link
Member

@DamirPorobic DamirPorobic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good and seems to work but the -1 as message I would replace with a dedicated signal.

Also something that I've noticed, when I fit the image to view, I get something like 116% zoom value and our zoom increments are 10% so I cannot get back to 100% without using the keyboard shortcut CTRL+0 which I had forgotten about myself, event though I'm the one who implemented it. What do you think about another button with corresponding signal that resets the view to 100% or do you think that this is overkill?

Any tests?

void ViewZoomer::setZoomValue(double value)
{
// ZoomPicker's mFitImageButton() sets value to -1
if (value < 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, this is a bit strange. When you think about it, the zoom value has not yet change but rather you want to send a notification from the zoom picker to the ViewZoomer and the ViewZoomer then does something that changes the zoom value and this new zoom value should be sent back to the zoom picker. So maybe a new signal for the zoom picker that the ViewZoomer listens to?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That means going trough AnnotationGeneralSettings, over AnnotationSettingsAdapter and land in AbstractSettingsProvider to access mZoomValueProvider :D

hold my beer

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, added as additional commit instead of --amending.

@precla
Copy link
Contributor Author

precla commented Feb 5, 2022

Looking good and seems to work but the -1 as message I would replace with a dedicated signal.

Ok, makes more sense.

Also something that I've noticed, when I fit the image to view, I get something like 116% zoom value and our zoom increments are 10% so I cannot get back to 100% without using the keyboard shortcut CTRL+0 which I had forgotten about myself, event though I'm the one who implemented it.

yeah, that sounds reasonable. Fixed so it reverts to 10% in/decrements on manual zoom change:
https://github.com/ksnip/kImageAnnotator/pull/281/files#diff-2130e12e9f11ef241e05aa6fb1aef112d7d530d5780bcd6bdcc9c2d79f5a681cR42

What do you think about another button with corresponding signal that resets the view to 100% or do you think that this is overkill?

Scrolling back to 100% is fast and easy. Wouldn't be a problem to add (and invoke my icon drawing skills again haha).
(another button just to get 10% steps would be too much imho)

Any tests?

Tested standalone and in Spectacle. Looks like it always misses by 1% when fitting in Spectacle.

@precla
Copy link
Contributor Author

precla commented Feb 5, 2022

Hm, build failing on github, working fine on my end. :s

@DamirPorobic
Copy link
Member

Looks like you have changed the ZoomProvider interface but not the Mock that implements it for the tests. I guess you're not building the tests local that's why it builds locally but not on the CI. You need to enable the test build, then you'll see it -DBUILD_TESTS=ON

@precla
Copy link
Contributor Author

precla commented Feb 7, 2022

done with Mock and added 100% , aka reset zoom button:

Screenshot_20220207_222514

@DamirPorobic DamirPorobic merged commit ba251a1 into ksnip:master Feb 15, 2022
@DamirPorobic
Copy link
Member

Sorry for the delay, merged. I think I'll have a look into the reset icon, it's a bit redundant to have 100% twice there, but for now it's fine. Thanks for providing this PR!

@precla
Copy link
Contributor Author

precla commented Feb 16, 2022

No problem. What other icon do you mean with reset? Can't see any other

@DamirPorobic
Copy link
Member

Not sure, maybe something like this?
image

Or maybe having a single button, like extended option and there something like a popup menu opens with "Fit to View" and "Reset". Just thinking aloud.

@precla
Copy link
Contributor Author

precla commented Feb 17, 2022

Hm, the rotating arrows remind me too much of the 'rotate image' button.
I'll think about it too, and respond if I manage to come up with some better.

@precla precla mentioned this pull request Mar 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants