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

Fix/standardize STANDOUT in wxWidgets builds #122

Merged
merged 4 commits into from
Dec 31, 2021

Conversation

dmalec
Copy link
Collaborator

@dmalec dmalec commented Dec 31, 2021

One possible approach to handling inverted text in wxWidgets builds (both STANDOUT and text selection highlighting).

This feels like it could potentially be refined, so I'm going to open it as a draft PR to allow for discussion and further experiments.

Test Environments

  • OSX Catalina (10.15.7) w/ wxWidgets 3.0.5
  • 
Ubuntu Bionic (18.04.5) w/ wxWidgets 3.0.5

  • Windows 10 Home (1909) w/ wxWidgets 3.0.5

@jrincayc
Copy link
Owner

Yes, that makes sense that that is the problem. Thank you for finding the cause. (As the comment in dc.h https://github.com/wxWidgets/wxWidgets/blob/master/interface/wx/dc.h makes clear wxINVERT doesn't work on Mac and GTK3).

I do wonder if it might be better to do (and I would have to look more at the code to figure out if this makes sense):

  //Flip the background and foreground colors
  dc.SetTextBackground(TurtleCanvas::colors[fg_color]); 
  dc.SetTextForeground(TurtleCanvas::colors[bg_color]);`
  //Draw inverted text
  ...
  //Switch back to original colors
  dc.SetTextBackground(TurtleCanvas::colors[bg_color]);
  dc.SetTextForeground(TurtleCanvas::colors[fg_color]);

@dmalec
Copy link
Collaborator Author

dmalec commented Dec 31, 2021

Good thought on flipping the foreground and background colors - I'll take a look at that approach.

@dmalec
Copy link
Collaborator Author

dmalec commented Dec 31, 2021

First cut at using foreground/background color inversion. Test code:

print standout "x
print (word "abc standout "def "ghi)
print "m 
settextcolor [100 64 2] [10 20 35]

Showing with default white on black color scheme:
Screen Shot 2021-12-31 at 10 01 42 AM

Showing with an amber on dark blue scheme:
Screen Shot 2021-12-31 at 10 02 06 AM

Showing how things look with the raw color inversion (since text selection is still using that approach):
Screen Shot 2021-12-31 at 10 02 18 AM

I like STANDOUT using the approach of flipping the foreground and background colors, good point there.

Any thoughts on text selection? I can see making the case either way - on the one hand, it's nice having it be visually distinct from STANDOUT since it's a distinct operation. On the other hand, since there's the established foreground//background, there's a nice consistency to sticking with them for color highlighting. Maybe selected text should be displayed in a standard high contrast foreground/background color that isn't impacted by the current color selection? I'm personally pulled a few different ways on this one...

@dmalec
Copy link
Collaborator Author

dmalec commented Dec 31, 2021

Possibly yes - I was thinking of the checks as minimizing calls to SetTextForeground and SetTextBackground; however:

  1. I am now thinking I was guilty of premature optimization
  2. Even so, I may have made the logic muddier than needed
  3. I now see the bug in my code that means I didn't even optimize things :)

@dmalec
Copy link
Collaborator Author

dmalec commented Dec 31, 2021

Refactored logic based on review. While testing, I noticed there's a bug in the way selection highlighting works if there is offscreen text in the terminal. I can try and sort that out on this PR or can pull that code back out and work it independently (now that STANDOUT is decoupled from text selection).

Copy link
Owner

@jrincayc jrincayc left a comment

Choose a reason for hiding this comment

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

Reviewed code, tested and I approve.

@jrincayc
Copy link
Owner

I am fine with merging as is, or splitting out the STANDOUT fix and merging that.

@dmalec
Copy link
Collaborator Author

dmalec commented Dec 31, 2021

Gotcha, I'm going to revert the changes to selection highlighting then - there seems to be a snarly bug there, so I'm going to need to mull a bit.

@dmalec dmalec changed the title Rough pass at fixing STANDOUT in wxWidgets Fix/standardize STANDOUT in wxWidgets builds Dec 31, 2021
@dmalec dmalec marked this pull request as ready for review December 31, 2021 17:34
Copy link
Owner

@jrincayc jrincayc left a comment

Choose a reason for hiding this comment

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

Reviewed, tested and approve

@jrincayc jrincayc merged commit a4e502d into jrincayc:master Dec 31, 2021
@jrincayc
Copy link
Owner

Thanks for fixing this :)

@dmalec
Copy link
Collaborator Author

dmalec commented Dec 31, 2021

You're welcome, no worries :)

@dmalec dmalec deleted the wip_standout_fix branch January 18, 2023 18:13
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.

2 participants