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

plugins: Allow plugins to customize their log colors #823

Merged
merged 10 commits into from
Oct 14, 2024

Conversation

uniboi
Copy link
Contributor

@uniboi uniboi commented Sep 12, 2024

If the color flag is 0 (expected but undocumented default behaviour of plugins for GetField) NS::Colors::PLUGIN is used as a default.

The Color is packed into a 64 bit integer like this

const Color = packed struct(u64) {
    red: u8,
    green: u8,
    blue: u8,
    _: u40 = 0,
};

The colors only occupy the lower 3 bytes of the integer. The remaining bits are ignored. When determining to use the default color or the provided log color, the lower 12 bits are checked if any is not 0.

Related: R2Northstar/NorthstarDiscordRPC#26

@github-actions github-actions bot added needs testing Changes from the PR still need to be tested needs code review Changes from PR still need to be reviewed in code labels Sep 12, 2024
@catornot
Copy link
Member

Were the plugin field and plugin string enums supposed to be non exhaustive?

@uniboi
Copy link
Contributor Author

uniboi commented Sep 12, 2024

yes to allow adding more fields in the future they're supposed to be non exhaustive

@catornot
Copy link
Member

Alright a few plugins crashing is fine for now ig
Will review this pr later today probably

primedev/plugins/plugins.h Outdated Show resolved Hide resolved
primedev/plugins/interfaces/IPluginId.h Outdated Show resolved Hide resolved
@F1F7Y F1F7Y added the waiting on changes by author Waiting on PR author to implement the suggested changes label Sep 15, 2024
Copy link
Member

@F1F7Y F1F7Y left a comment

Choose a reason for hiding this comment

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

The above review wasnt supposed to be an approval smh

@uniboi uniboi requested a review from F1F7Y October 2, 2024 12:08
@uniboi uniboi removed the waiting on changes by author Waiting on PR author to implement the suggested changes label Oct 2, 2024
Copy link
Member

@catornot catornot left a comment

Choose a reason for hiding this comment

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

Looks good me thinks

also pretty colors for plugins :)
image

@catornot catornot removed the needs testing Changes from the PR still need to be tested label Oct 3, 2024
@GeckoEidechse
Copy link
Member

GeckoEidechse commented Oct 4, 2024

Yeah so for some reason it renders as (almost?) black when the colour value is not set which looking at the code shouldn't be the case as it should just be default colour if not set

Screenshot is from when running the PR with DiscordRPC at v12, i.e. current
image

@uniboi
Copy link
Contributor Author

uniboi commented Oct 6, 2024

Current rrplug builds return uninitialized values for colors. If a plugin returns 0 as color, a default will be used

@GeckoEidechse GeckoEidechse changed the title allow plugins to customize their log colors plugins: Allow plugins to customize their log colors Oct 7, 2024
Copy link
Member

@GeckoEidechse GeckoEidechse left a comment

Choose a reason for hiding this comment

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

Confirmed working in testing together with R2Northstar/NorthstarDiscordRPC#26 :D

image

Co-authored-by: GeckoEidechse <40122905+GeckoEidechse@users.noreply.github.com>
@GeckoEidechse
Copy link
Member

cc @F1F7Y whether you wanna re-review, given that you requested changes. Otherwise if there's no response I'd merge within a day or two to not stall the PR o7

Copy link
Member

@F1F7Y F1F7Y left a comment

Choose a reason for hiding this comment

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

Code looks acceptable

@GeckoEidechse GeckoEidechse merged commit 27e1711 into R2Northstar:main Oct 14, 2024
4 checks passed
@GeckoEidechse GeckoEidechse removed the needs code review Changes from PR still need to be reviewed in code label Oct 14, 2024
@uniboi uniboi deleted the plugin-log-colors branch October 15, 2024 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants