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

Implement new 'frame' parameter in 'info' method #45

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

oaubert
Copy link

@oaubert oaubert commented Mar 4, 2013

It returns values (in percentage) for displayed area of the image.
This allows to implement the drawing of the displayed area in a thumbnail for instance.

It returns values (in percentage) for displayed area of the image.
This allows to implement the drawing of the displayed area in a thumbnail for instance.
@can3p
Copy link
Collaborator

can3p commented Mar 5, 2013

Did you get into account the rotation? For example, see how is orig_height and orig_width are implemented in the info method. The second thing is that it seems that this method is not enough to draw thumbnail window, because you need to update it after every change of image zoom or position. The last question is - why are you passing zoom value here? You can already retrieve zoom from info method.

@oaubert
Copy link
Author

oaubert commented Mar 5, 2013

Indeed, I did not take rotation into account. It seems an exotic use case for me anyway, but I guess it could be addressed by adding the angle information to the returned structure, and letting the user process it the way he wants.

For the update question, I currently invoke it in a method that is bound to the onAfterZoom and onStopDrag events, because I do not want continuous update. It could also be used in methods bound to onZoom/onDrag for continuous updates.
Of course, another approach could be to implement it as a new event (or maybe 2: onVisibleFrameUpdate for continuous updates and onAfterVisibleFrameUpdate for non-continuous updates), but I first wanted to implement the feature with minimal changes.

Lastly, for the zoom information, I do not use it for the moment but I guess it could be a potentially useful information in this context, and the cost of putting it into this structure is far less than an additionnal call to the info method.

@oaubert
Copy link
Author

oaubert commented Mar 5, 2013

I have just added an example file so that you can experiment with this new feature.

@can3p
Copy link
Collaborator

can3p commented Mar 5, 2013

Can you update the code to handle the rotation properly? I think that thumbnailer is a great feature the can fit nicely into the widget core, however it should react on all properly in all corner cases. The reason why this should be implemented is uniformity of the API. All the public methods switch width and height depending on the angle, so that user can avoid implementing this logic by himself.

@oaubert
Copy link
Author

oaubert commented Mar 5, 2013

Honestly, not seeing a valid use case for this kind of setup (a 90 degree rotated image with a non-rotated thumbnail?), I cannot properly define, hence implement, rotation handling. Do you have concrete examples of rotation uses to make things clearer?

@can3p
Copy link
Collaborator

can3p commented Mar 6, 2013

No, it's the thing I'm trying to avoid.
E.g. if in non rotated state the function return { x: 0.1, y: 0.2, w: 0.3, h: 0.4 } then the values cannot be the same if we rotate the image 90 degrees. Later this day I will try to fix it in code and push it to your fork, so all the changes could be in one place.

@Silex
Copy link
Contributor

Silex commented Dec 4, 2013

Ah, this is very nice, I was looking about how to implement a "minimap" for the current image, and this seems to solve it. Any chance to get this merged soon or should I use @oaubert fork?

@can3p
Copy link
Collaborator

can3p commented Dec 5, 2013

@Silex we still need to achieve a consistent behaviour in cases when image is rotated. Could you look into this? If we fix this corner case I'll merge this in the master

@Silex
Copy link
Contributor

Silex commented Dec 6, 2013

I'll look into it... but I'm not sure it make sense to have a minimap for rotated images. I think it makes more sense for the minimap to be rotated as well?

@can3p
Copy link
Collaborator

can3p commented Dec 6, 2013

Yes, sure. I think that minimap should look the same as main image. So if our image is rotated then it makes no sense to have the minimap that is not rotated

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.

3 participants