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

I3908 image rotation, flip and invert #4000

Merged
merged 24 commits into from Mar 15, 2018
Merged

I3908 image rotation, flip and invert #4000

merged 24 commits into from Mar 15, 2018

Conversation

@cmprince
Copy link
Contributor

@cmprince cmprince commented Feb 25, 2018

This PR addresses the image widget rotation issue #3908. This adds css to center the image on the page so that the rotation transformation origin can be set to the image center. It also assigns "[" and "]" as keyboard shortcuts.

Copy link
Member

@ian-r-rose ian-r-rose left a comment

This is looking great, thanks @cmprince! I have a couple of very minor comments above.

My biggest question: when I open an image, the initial scale now appears smaller than it did before, not taking up the whole tab:
image
Is it possible to tweak the CSS so the initial scale is larger?

@@ -25,6 +25,12 @@ namespace CommandIDs {

export
const zoomOut = 'imageviewer:zoom-out';

export
const rot90 = 'imageviewer:rot90';
Copy link
Member

@ian-r-rose ian-r-rose Feb 25, 2018

Maybe we could name the command IDs imageviewer:rotate-clockwise and imageviewer:rotate-counterclockwise for better human-readability?

@@ -134,7 +140,19 @@ function addCommands(app: JupyterLab, tracker: IImageTracker) {

commands.addCommand('imageviewer:reset-zoom', {
Copy link
Member

@ian-r-rose ian-r-rose Feb 25, 2018

Also here: maybe update the name of the command ID in addition to the label?

const widget = tracker.currentWidget;

if (widget) {
widget.rotation += 90;
Copy link
Member

@ian-r-rose ian-r-rose Feb 25, 2018

I don't think it ultimately matters too much, but I like to take the modulus of angles by 360 so that they are clearer when inspected.

Copy link
Contributor Author

@cmprince cmprince Feb 25, 2018

Thanks for the comments @ian-r-rose! I'll have to figure out why initial scale is changing, but the rest are no-brainers.

Copy link
Contributor Author

@cmprince cmprince Feb 25, 2018

Removing max-width: 100% and max-height: 100% in the CSS seems to fix the scaling issue. I don't know if that breaks something else, but I don't see any problems so far.

@cmprince cmprince changed the title I3908 image rotation I3908 image rotation, flip and invert Feb 25, 2018
Copy link
Member

@ian-r-rose ian-r-rose left a comment

I noticed some odd behavior of the rotated image, whereby it is showing above the microtoolbar of the tab. We may need to mess with the z-orders a bit.

"imageviewer:rot90": {
"default": { },
"properties": {
"command": { "default": "imageviewer:rot90" },
Copy link
Member

@ian-r-rose ian-r-rose Feb 26, 2018

These commands should be updated to reflect the new names.

}


.jp-ImageViewer img {
max-width: 100%;
max-height: 100%;
display: table-cell;
Copy link
Member

@ian-r-rose ian-r-rose Feb 26, 2018

Are these CSS properties (display, vertical-align, text-align) necessary here? I don't quite understand what they are meant to do.

Copy link
Contributor Author

@cmprince cmprince Feb 26, 2018

They were left over from getting the image to center properly. The div attributes to that now, so these aren't necessary.

@cmprince
Copy link
Contributor Author

@cmprince cmprince commented Feb 26, 2018

I'm not sure how to fix the microtoolbar issue above. It's seen in both color schemes. I tried playing with the z-order in the ::before section of the css, but didn't have any effect. I'd appreciate any advice from CSS experts!

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Feb 26, 2018

Very nice! I love these extra capabilities.

I also see the scrollbars appearing.

Also, the [ and ] seem to do the opposite of what I expected - I expected ] to rotate the image clockwise and [ to rotate counter-clockwise (since the ] is on the right and [ is on the left in the US keyboard).

Also, it seems that most UIs in my very cursory search in different programs say "rotate left" and "rotate right" rather than "clockwise" and "counter-clockwise".

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Feb 26, 2018

Also, for comparison, Preview on macOS has Cmd L for rotate left, Cmd R for rotate right. Adobe Lightroom has Cmd [ for left, Cmd ] for right. Windows photo viewer has Ctrl , for left, Ctrl . for right. In either case, the key on the left rotates left, the key on the right rotates right.

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Feb 26, 2018

I was able to fix the microtoolbar issue by adding z-index: -1 to the .jp-ImageViewer > div selector. I'm not sure why it wasn't working on the :before element.

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Feb 26, 2018

I agree with @jasongrout, the default key strokes for rotate are inverted from what I would expect.

I actually think that Clockwise and Counterclockwise are clearer than Right and Left. After all, when something is rotating, part of it is going left, and part of it is going right.

These minor things aside, this PR is looking really good.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Feb 26, 2018

I'm looking at the code a bit more too. I'm curious: why do we have a wrapper div element around the image? It seems like we have a lot of 50% transforms because we are applying the transformations to a wrapper div instead of the image itself.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Feb 26, 2018

Also, for another PR - it seems that the scale commands aren't quite reversible. From the default zoom, pressing + - doesn't get you back to the normal zoom, since the scaling behavior seems to change if the scale is greater than 1.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Feb 26, 2018

I'm curious: why do we have a wrapper div element around the image?

Ah, I see better. It seems that browsers don't consider a transformed image as necessarily bigger, so if you zoom in, for example, you can't scroll around the image (i.e., the overflow scroll isn't expanded so that you can scroll around the entire image).

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Feb 26, 2018

@jasongrout I, too, am curious why the scaling behavior is the way it is (though I agree that changing it is outside of the scope of this PR). I might expect a constant scaling factor (of maybe ~1.3, or perhaps e or \phi?) so that it would be easily undoable.

@@ -215,15 +215,15 @@ function addCommands(app: JupyterLab, tracker: IImageTracker) {
const widget = tracker.currentWidget;

if (widget) {
widget.rotation += 90;
widget.rotation -= 90;
Copy link
Contributor

@jasongrout jasongrout Feb 26, 2018

Don't we want "clockwise" to still mean clockwise, and instead switch which command is tied to the [ key? I think this change makes the clockwise and counter-clockwise commands do the opposite of what they advertise.

@cmprince
Copy link
Contributor Author

@cmprince cmprince commented Feb 26, 2018

Thanks @jasongrout for catching the CW/CCW flip. I thought I noticed that too, but I managed to convince myself it was OK; I should listen to my family and go to bed earlier 😄 Thanks @ian-r-rose for the z-order fix! I only recently got a crash course in CSS so the help is appreciated.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Feb 27, 2018

I'm happy to have you push your commits on top if you have something in mind!

I'm working on something I'll be pushing soon.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Feb 27, 2018

Taking up the right/left vs cw/ccw debate:

I think there is pretty clear precedent for right/left. How about the lightroom approach, Right (CW), Left (CCW)?

jasongrout added 2 commits Feb 28, 2018
We’ll remove this until we (a) have a nice looking indicator, and (b) have a good command for hiding/showing it.

Perhaps when we have a toolbar, we should just put the status in the toolbar?
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Feb 28, 2018

@cmprince, @ian-r-rose - what do you think? I did do a rename of the clockwise/ccw -> right/left, since it felt like "counterclockwise" was too long, and "cw" and "ccw" are too easy to confuse. If there are strong feelings the other way, I can revert that part.

Also, I think images are no longer centered in the pane. Not sure if they were before, but the rotation stuff works out better I think to put the image in the upper left.

I also added and then removed an orientation indicator. The one I had looked dumb, but the code is now in the repo so that it's easy to add back when we get a decent indicator.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Feb 28, 2018

I changed the behavior back to not automatically sizing to the pane size. Perhaps that is better. Perhaps we can also have a "F" for fit - where that sets the max-height and max-width CSS removed in 870ad72

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Feb 28, 2018

I still have a preference for "Clockwise" and "Counterclockwise" (and your list doesn't seem like it has that clear of a precedent). That said, I don't think we need to block on that decision.

I like the matrix approach, and think this is in really good shape. Thanks @cmprince and @jasongrout!

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Mar 1, 2018

Sorry, I meant to say "a clear precedent"…as in it was in common use, not that it was clearly the winner from my examples.

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Mar 1, 2018

Ah, sorry, I misunderstood. In any event, I don't really mind Right/Left, and have seen them in a bunch of applications. Usually when I want to rotate an image, I just mash the keys until it looks right anyway 😄

@cmprince
Copy link
Contributor Author

@cmprince cmprince commented Mar 1, 2018

I am partial to clockwise/counterclockwise since it's unambiguous, but I agree that "left/right" is fairly common as well. I wonder if those directions used in non-English locales as well. I wouldn't feel strongly one way or the other if both are commonly used elsewhere.

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Mar 1, 2018

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Mar 13, 2018

@cmprince @jasongrout What is the status of this? I think it's in pretty good shape, pending a decision on Right/Left vs Clockwise/Counterclockwise. I don't really want to let it languish for too long.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Mar 13, 2018

Thanks for the ping - I was going to switch it back to CW/CCW given the extra vote on that side. Other than that, I think it's good.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Mar 13, 2018

@cmprince - do you want to switch the code back to using CW/CCW?

@cmprince
Copy link
Contributor Author

@cmprince cmprince commented Mar 14, 2018

@jasongrout Just done! I tried to modify the tests in tests/test-imageviewer for rotation and the other features, but I don't think I quite have the hang of it. I can push what I have, but if it's not necessary I (or someone else) can add it in another PR.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Mar 14, 2018

@cmprince - Thanks! For consistency, can you also change rotateLeftMatrix and rotateRightMatrix (definition and where they are used)?

Edit: I meant change the names to rotateClockwiseMatrix and rotateCounterclockwiseMatrix...

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Mar 15, 2018

Thanks! I tested one last time, and it seems to work great.

@jasongrout jasongrout merged commit 8de8cf6 into jupyterlab:master Mar 15, 2018
2 checks passed
@jasongrout jasongrout added this to the Beta 2 milestone Mar 15, 2018
@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Mar 15, 2018

Thanks for the hard work @cmprince and @jasongrout!

@hamlet82
Copy link

@hamlet82 hamlet82 commented Mar 16, 2018

Thank you all! Look forward to seeing it in action. This will be a huge help.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Mar 16, 2018

Great! We're working hard to get the next beta out.

@cmprince cmprince deleted the i3908-image_rotation branch Mar 24, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Aug 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants