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

Add EEPROM and focus support to ColormapOverlay #1434

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

EvyBongers
Copy link
Contributor

Quick attempt to implement focus support in ColormapOverlay. I feel this should work, but I don't have my keyboard with me right now, so any debugging I'll have to do later.

@@ -73,6 +74,10 @@ EventHandlerResult ColormapOverlay::beforeSyncingLeds() {
return EventHandlerResult::OK;
}

EventHandlerResult ColormapOverlay::onFocusEvent(const char *input) {
return ::LEDPaletteTheme.themeFocusEvent(input, PSTR("colormap.overlay"), map_base_, no_themes_);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

colormap.overlay seems like the most logical focus command to me. Let me know what you think.

@obra
Copy link
Member

obra commented Jul 29, 2024

@EvyBongers did you end up testing this on hardware?

@EvyBongers
Copy link
Contributor Author

I completely forgot about this. I'll see about testing it today

@EvyBongers
Copy link
Contributor Author

EvyBongers commented Nov 13, 2024

So, I finally got around to testing this. The command returns the palette indexes as expected, but setting the indexes doesn't yet work 🤔

@EvyBongers EvyBongers marked this pull request as draft November 13, 2024 14:35
@EvyBongers
Copy link
Contributor Author

So, I dug a little deeper to see what I was missing. I thought I had a quick fix by calling on ::LEDPaletteTheme.themeFocusEvent(), but the internal logic of the plugin never actually used LEDPaletteTheme to store its data because it was never intended to work as a theme.
Looking at #1416, it seems to me that the third PR that we discussed there was never created, so unless we want so implement a read only version here, this effort is going to have to wait until EEPROM support is added to Colormap-Overlay.

The confusing part is that Colormap-Overlay (seemed to) need to call ::LEDPaletteTheme.reserveThemes() in order to work with the palette, even though it never uses any of the theme logic. It's been ages since I implemented that part and I don't remember why I needed to add that call to reserve themes nor whether it's actually needed for the plugin to work with the palette at all.

Closing this for now

@EvyBongers EvyBongers closed this Nov 13, 2024
@EvyBongers EvyBongers reopened this Nov 13, 2024
@EvyBongers
Copy link
Contributor Author

Reopened because EEPROM supports needs to be added anyway and EEPROM and focus support should land together.

@EvyBongers EvyBongers changed the title Add focus support to ColormapOverlay Add EEPROM and focus support to ColormapOverlay Nov 13, 2024
@EvyBongers
Copy link
Contributor Author

Just found a comment about this on Discord. Cross-posting to have all of this together.

#1434 will need quite a bit of work yet, I think. My first attempt to make focus work for colormap overlay settings simply passed through the handling of the focus event to ::LEDPaletteTheme.themeFocusEvent(). I had a proper look at that today and I'm pretty sure that isn't going to work, or at least it isn't sufficient. That's partly because (iirc) I needed to reserve at least one theme to get ::LEDPaletteTheme.lookupPaletteColor() to work. However, the plugin doesn't store any other data in a theme. That means that I will either have to find a way to have the plugin work with themes or I'm gonna have to implement a second or different means for handing the focus command

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