Skip to content

fw: black notification banner with inverted icon on black and white watches #159

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jplexer
Copy link

@jplexer jplexer commented Apr 20, 2025

small change to make notification banners on bw watches a bit nicer
(design wise signed off by @lavglaab )

image

@jplexer jplexer force-pushed the jplexer/silk_notif branch from fba1108 to 0e48841 Compare April 20, 2025 22:42
@jplexer
Copy link
Author

jplexer commented Apr 20, 2025

@HBehrens I have changed the layer of abstraction in comparison to #52, would love your feedback as the "pdc" expert :)

@HBehrens
Copy link

I saw your comment and put it on my list for Tuesday.

@jwise
Copy link

jwise commented Apr 22, 2025

We really need to make clang-format stop making so much noise. I like this, though.

Copy link

@HBehrens HBehrens left a comment

Choose a reason for hiding this comment

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

I am skeptical about the (ab-)use of the context pointer to capture state – please see my inline comments for the reasoning and an alternative proposal.

Made a few other minor suggestions.

Would love to see tests for the inversion behavior!

.command = prv_invert_pdc_colors,
};

KinoReelProcessor invert_processor = {.draw_command_processor = &gdraw_inv_processor};

Choose a reason for hiding this comment

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

This will similarly require values for bitmap_processor. Of course, this could happen at a follow-up PR. My recommendation would be to not solve with via GBitmapProcessor.pre or .post but a new field composite_mode for a lot of orthogonal power.

@jplexer jplexer force-pushed the jplexer/silk_notif branch from 0e48841 to c150bd4 Compare April 22, 2025 12:12
@jplexer
Copy link
Author

jplexer commented Apr 22, 2025

did most of the things you pointed out @HBehrens, will look into tests and inversion logic for full color soon!

@jplexer jplexer force-pushed the jplexer/silk_notif branch from c150bd4 to 7632efc Compare April 23, 2025 17:26
@jplexer
Copy link
Author

jplexer commented Apr 23, 2025

rebased to remove all the clang-format changes and make more obvious what my changes are :)

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.

3 participants