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

Darkmode #506

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Darkmode #506

wants to merge 7 commits into from

Conversation

pdfinn
Copy link

@pdfinn pdfinn commented Aug 18, 2024

I know this to be sacrilegious, but nevertheless, I've made some additions to enable some colour scheming — primarily to implement a 'dark mode'. It is set at startup using a -v flag (for Vampira). Colour values are currently hard-coded in theme.go. Orthodox colours remain are set by default.

@rjkroege
Copy link
Owner

I'm open to the idea of dark mode (or just configurable colours.) Can I persuade you to make some small code changes before merging?

@pdfinn
Copy link
Author

pdfinn commented Aug 28, 2024

Certainly, what code changes did you have in mind?

@rjkroege
Copy link
Owner

rjkroege commented Sep 8, 2024

I'll write some review comments. Sorry for the delay.

Copy link
Owner

@rjkroege rjkroege left a comment

Choose a reason for hiding this comment

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

Hi!

I have resolved that in 2025, Edwood will not be in a coma. Sorry for the delay. I have several comments on your CL.

High level

I (in general) like the ideas of configurable colours but I had imagined doing it more ambitiously:

  • load/store the colours from the dump file configuration
  • stretch make the colours configurable from the filesystem

But your approach seems like a good start towards my more ambitious ideas. I hope consequently that you'd be willing
to address my comments in a subsequent pushes to your PR.

Specific Comments

  • acme.go:7 Can you group all the Go first-party and third-party imports separately like in the other files?
  • acme.go:35 I'd prefer a longer flag name like -dark.
  • frame/draw.go:218 Why not have a theme object passed into libframe on init? The theme object would
    specify the colours. libframe should not need to know about themes; only what colours to use.
  • frame/draw.go:230 Please keep my original comment. It's to remind me to fix something eventually.
  • frame/tick.go:4 Sort the imports per my previous comment
  • frame/tick.go:11 I missed seeing where you call drawTick. Remove it if unused.
  • frame/tick.go:41 Same issue as above; I think that libframe should only care about colours. Themes are
    the province of your (rightly) new theme module.
  • debug logging can be removed. Yes, I've been bad and not done this everywhere but no need to make it worse.
  • frame/tick.go:68 Flushes are very expensive. I haven't seen an issue needing this in my build. What were you seeing?
  • globals.go:158 Globals also could just care about colours? The theme module could perhaps provide a colour table in dark or light?
  • globals.go:178 There's a refactoring opportunity with iconinit right? Particularly given the theme module.
  • theme/theme.go:4 Sort the imports
  • theme/theme.go:21 Put the colours in a struct? Have semantic colour fields. Use those everywhere. The
    theme module should configure the semantic colours. I.e.: rather than Paleyellow, have Tagbackground, Textbackground etc. and these are set to dark or light variations as appropriate.
  • theme/theme.go:59 Why have key bindings code here here? I wouldn't consider it part of colour theme support.

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