-
Notifications
You must be signed in to change notification settings - Fork 1.3k
show crop aspect ratio in crop preview #20191
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
base: master
Are you sure you want to change the base?
Conversation
|
It would be good in the future to raise an issue first then do the PR. This takes care of the case where the "issue" gets "worked" in pixls.us instead of in the darktable repository. I'm all for pixls.us as a breeding ground for ideas, but I think the workflow needs to be pixls.us -> issue -> pull request. This gives devs a chance to chime in/buy in and it prevents "feature creep" from pixls.us. @deekayhd I'm not criticizing you and I'll make a post in pixls.us with the above. See #20123. |
|
@wpferguson I get your point. Actually, I know your proposal in #20123, but I misunderstood it, as it mentions only fixes. But I understand that it is desirable for feature requests, as well. |
You're right, it does read that way. I'll fix it. Thanks. |
|
I think the display of the crop properties would be a nice enhancement, but it's position in the center can be distracting when trying to get an impression of the cropped image. Maybe e. g. moving it up, so it touches the upper image boundary - while still be centered horizontally -, would be less intrusive? Also I think the grey background is a little large with respect to the text. |
This PR is just to add text to existing functionality. If the placement needs to be changed, then that should be a separate issue for discussion with a PR to follow. |
@rgr59 I do not think this is a problem. When you release the left mouse button, the dimensions and ratio vanish. |
@wpferguson Wouldn't that be too much overhead for the process? The requirement, as also described in the RFC #20192, is to show the aspect ratio while cropping. It was an initial suggestion to put it next to the dimensions, but discussing the position would still be part of the specification for this one, IMO. |
|
Right now crop appears in the center and everyone is used to it appearing in the center, correct? If you change the crop display from the center to somewhere else, you've changed the behavior for all users without any heads up. Also the issue says middle of the screen. EDIT: Or worse yet left the crop dimensions in the center and put the aspect ratio somewhere else so you'd need 3 eyes to keep track of everything (dimensions, ratio, image). |
|
I think I misunderstood the comment to relate to the aspect ratio, only. I also would not want to split the display or move everything elsewhere with this PR. Sorry for the noise. |
Sure, and while resizing the display does not disturb much. More so when after resizing you move the crop around to find the best place for it. Maybe the display can be suppressed while moving, and shown only when the mouse button is pressed while being over a resize handle? |
|
As wpferguson already commented, we should discuss this kind of change in a separate issue/PR, because it has a greater impact on the UX than simply adding the additional ratio. |
This enhancement proposal originated from a suggestion on the pixls.us forum by user elGordo (Displaying freeform crop aspect ratio). See also feature request #20192

In the crop module when adjusting the crop, the dimension of the crop area are displayed as x in the middle of the frame. The proposal is to also show the aspect ratio of the crop area, as shown in the screenshot: