jrincayc / ucblogo-code

Berkeley Logo interpreter
https://people.eecs.berkeley.edu/~bh/logo.html
GNU General Public License v3.0
187 stars 34 forks source link

Fix/standardize STANDOUT in wxWidgets builds #122

Closed dmalec closed 2 years ago

dmalec commented 2 years ago

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

jrincayc commented 2 years ago

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 commented 2 years ago

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

dmalec commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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).

jrincayc commented 2 years ago

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

dmalec commented 2 years ago

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.

jrincayc commented 2 years ago

Thanks for fixing this :)

dmalec commented 2 years ago

You're welcome, no worries :)