Skip to content
New issue

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

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

Already on GitHub? # to your account

Make previews always fit vertically in the preview area #944

Merged

Conversation

SamCarlberg
Copy link
Member

@SamCarlberg SamCarlberg commented Apr 25, 2019

No more previews clipping below the bottom that forced users to scroll up and down to see information displayed below the image (eg contour count)

Reimplement TitledPane and ImageView to get proper behavior. JavaFX TitledPane leaves a gap around the image when it's shrunk, and ImageView is atrocious at resizing to fit its parent.

Remove the overloads in ImageConverter that take desired height, since the UI is now responsible for doing the resize calculations.

This is a better implementation of #711 and fix for #710

Screenshots

Full size previews
GRIP Computer Vision Engine | Edited_012

Partially shrunk
GRIP Computer Vision Engine | Edited_014

Minimum size previews
GRIP Computer Vision Engine | Edited_015

No more previews clipping below the bottom, forcing users to scroll up and down to see information displayed below the image (eg contour count)

Reimplement TitledPane and ImageView to get proper behavior
@JLLeitschuh
Copy link
Member

Ooof. Not 100% sure I'm on board with this change.

When I resize an image, I kinda want the image itself to be the same size across the entire preview pane. I really don't care that much about the size of the outer box.

What's the reasoning here? Do you really think it's a better UX experience?

@SamCarlberg
Copy link
Member Author

The problem with the current behavior is that extra information is hidden below the scrollpane divider, even though the images are shrunk. This makes it difficult, for example, to see the number of contours when the preview area is vertically small. The current behavior does shrink the images so they're (usually) always entirely visible, but this PR makes the entire preview always visible.

@JLLeitschuh
Copy link
Member

@AustinShalit @JacisNonsense @bradamiller Seeking feedback on the UI changes in this PR.

I just want to make sure that we get solid feedback from users.

@bradamiller
Copy link
Member

@JLLeitschuh I'm not sure what the alternative would be since the information below the image (in this case, the number of contours) needs to also be visible. To keep all the images the same size, we would have to shrink all of them, leaving white space on the others to match the space taken by the text.

@JLLeitschuh
Copy link
Member

To keep all the images the same size, we would have to shrink all of them, leaving white space on the others to match the space taken by the text.

That's an interesting thought, although, I don't think it would look good visually.

I guess the question is, is it more important to have consistent sizes in images or is the consistent size of the preview panes more important.

I don't really have an answer to this question.

@Octogonapus
Copy link
Member

@JLLeitschuh My thoughts are the same as @bradamiller's. If you want all the images to be the same size, then boxes with extra info will cause a gap. I think it looks fine as-is.

@AustinShalit
Copy link
Member

I think this change looks good. My brain is ok dealing with the scaling difference when looking at the images and I would rather this than part of the image always being cut off.

@JaciBrunning
Copy link
Member

Looks good to me, should help with those tiny KoP screens.

Copy link
Member

@JLLeitschuh JLLeitschuh left a comment

Choose a reason for hiding this comment

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

LGTM!

@SamCarlberg SamCarlberg added this to the v1.6.0 milestone May 16, 2019
@SamCarlberg SamCarlberg merged commit 69f5704 into WPIRoboticsProjects:master May 16, 2019
@SamCarlberg SamCarlberg deleted the better-preview-resizing branch May 16, 2019 19:15
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants