FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

STY: Update macosx zoom rect styling by greglucas · Pull Request #24367 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

STY: Update macosx zoom rect styling - #24367

Merged
tacaswell merged 1 commit into
matplotlib:mainfrom
greglucas:macosx-zoom-box-style
Dec 13, 2022
ConversationCommits (1)ChecksFiles changed
Merged

STY: Update macosx zoom rect styling#24367
tacaswell merged 1 commit into
matplotlib:mainfrom
greglucas:macosx-zoom-box-style

Conversation

greglucas commented Nov 4, 2022

Copy link
Copy Markdown
Contributor

PR Summary

Match the zoom box styling of the other backends with alternating white/black paths.

closes #23777

PR Checklist

Tests and Styling

  • [N/A] Has pytest style unit tests (and pytest passes).
  • [N/A] Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • [N/A] Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • [N/A] New plotting related features are documented with examples.

Release Notes

  • [N/A] New features are marked with a .. versionadded:: directive in the docstring and documented in doc/users/next_whats_new/
  • [N/A] API changes are marked with a .. versionchanged:: directive in the docstring and documented in doc/api/next_api_changes/
  • [N/A] Release notes conform with instructions in next_whats_new/README.rst or next_api_changes/README.rst

greglucas added this to the v3.7.0 milestone Nov 4, 2022
greglucas marked this pull request as ready for review November 4, 2022 20:16
QuLogic reviewed Nov 5, 2022
View reviewed changes
Comment thread src/_macosx.m Outdated
greglucas force-pushed the macosx-zoom-box-style branch from 24dd283 to 50f25ed Compare November 5, 2022 15:55
QuLogic reviewed Nov 8, 2022
View reviewed changes
Comment thread src/_macosx.m Outdated
// in phase.
NSBezierPath *white_path = [NSBezierPath bezierPathWithRect: rubberband];
NSBezierPath *black_path = [NSBezierPath bezierPathWithRect: rubberband];
double dash_pattern[2] = {3, 3};

QuLogic Nov 8, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Should be CGFloat technically.

Suggested change
double dash_pattern[2] = {3, 3};
CGFloat dash_pattern[2] = {3, 3};

Comment thread src/_macosx.m
{
if (!NSIsEmptyRect(rubberband)) { [self setNeedsDisplayInRect: rubberband]; }
// The space we want to redraw is a union of the previous rubberband
// with the new rubberband and then expanded (negative inset) by one

QuLogic Nov 8, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Just checking if this is in logical space or physical space (i.e., should be one or one * display ratio)?

greglucas Nov 8, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

On HiDPI, this still clears the region and I'm not sure we want to bring in the deviceScale/contextRegion if we don't absolutely need to here. The linewidth is drawn in points and centered on the rectangle area, so the default linewidth of 1 expands 0.5 points on either side of the rect. (linewidth section from here
https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/CocoaDrawingGuide/Paths/Paths.html#//apple_ref/doc/uid/TP40003290-CH206-BBCDEJGD)

oscargus commented Nov 8, 2022

Copy link
Copy Markdown
Member

Two things (I cannot test or judge the code...):

  1. The (some) other backends draw one solid line and one dashed on top. It may be worth checking if that improves or degrades performance. (It is primarily done because it was hard to get an offset in those, though.)
  2. This can possibly go in 3.6.3 as it is in some sense a bug fix. The other backends went into 3.6.2.

STY: Update macosx zoom rect styling
cb68ffd 
Match the zoom box styling of the other backends with alternating
white/black paths.
greglucas force-pushed the macosx-zoom-box-style branch from 50f25ed to cb68ffd Compare November 8, 2022 15:42

greglucas commented Nov 8, 2022

Copy link
Copy Markdown
Contributor Author

The (some) other backends draw one solid line and one dashed on top. It may be worth checking if that improves or degrades performance. (It is primarily done because it was hard to get an offset in those, though.)

This is already so fast I don't think it is worth worrying about performance. I also don't want to overlap if we can avoid it so we don't have any additional weird aliasing / blending going on for the overlapping regions.

This can possibly go in 3.6.3 as it is in some sense a bug fix. The other backends went into 3.6.2.

I see this as an enhancement/improvement and not a bugfix. I really don't think these should be backported, but rather if we want it in sooner, a new minor release should happen sooner. The cursor styling and other aesthetic changes like that should be in minor versions not patch IMO.

oscargus commented Nov 8, 2022

Copy link
Copy Markdown
Member

OK! Makes sense. I was under the impression that something had changed in recent versions, but maybe it is just a consequence of more and more people using dark themes that we get those bug reports now...

greglucas commented Dec 12, 2022

Copy link
Copy Markdown
Contributor Author

ping for a macosx user to review this. It'd be nice to get in for the 3.7 release to keep the backends consistent in their styling.

jklymak approved these changes Dec 12, 2022
View reviewed changes

jklymak left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This works for me. The dashes are a bit small on my hiDPI screen compared to a zoom rectangle in Preview.app, but not crazy different. I'm a little surprised there is not an API for a rubberband already, but this seems to work.

tacaswell commented Dec 13, 2022

Copy link
Copy Markdown
Member

Confirmed that this looks good axes with white, black, and "0.5" patch colors.

tacaswell merged commit e0d65b6 into matplotlib:main Dec 13, 2022
greglucas deleted the macosx-zoom-box-style branch December 13, 2022 00:10

greglucas commented Dec 13, 2022

Copy link
Copy Markdown
Contributor Author

The dashes are a bit small on my hiDPI screen compared to a zoom rectangle in Preview.app, but not crazy different

I think the lines should be the same thickness as before, just dashed now. It is easy enough to increase the thickness if desired too. On preview for me, the dash pattern actually rotates around.

jklymak commented Dec 13, 2022

Copy link
Copy Markdown
Member

On preview for me, the dash pattern actually rotates around.

Yes, but I wasn't going to ask you to do that. However, that is what made me think there must be a selector API

QuLogic commented Dec 13, 2022

Copy link
Copy Markdown
Member

On preview for me, the dash pattern actually rotates around.

What do you mean by rotates around?

jklymak commented Dec 13, 2022

Copy link
Copy Markdown
Member

The dash pattern precesses around the selection in a clockwise manner

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

QuLogic QuLogic left review comments
jklymak jklymak approved these changes

Assignees

No one assigned

Labels

GUI: MacOSX

Projects

None yet

Milestone

v3.7.0

Development

Successfully merging this pull request may close these issues.

[ENH] Interactive Zoom Rectangle Color Review for MACOSX backend

5 participants

Back | FazBrowse Home | New Git URL