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.

Sign up for GitHub

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

Jump to bottom

refactor/improve monitor mode selection #8804

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

refactor/improve monitor mode selection #8804

wants to merge 11 commits into from
+384 −425

Conversation

ikalco
Copy link
Contributor

@ikalco ikalco commented Dec 22, 2024 •
edited
Loading

Describe your PR, what does it fix/add?

aims to

  • always ensure a mode is selected, if requested fails still set one, i might add an error notification
  • clarify mode selection code
  • consistent logs

also fixes format selection trying to use INVALID

Is there anything you want to mention? (unchecked code, possible bugs, found problems, breaking compatibility, etc.)

Is it ready for merging, or does it need work?

yes probably
maybe some other people should test too, idk who tho

@ikalco ikalco force-pushed the improve_monitor_rule_mode_selection branch from 9980f8a to f527bc4 Compare December 22, 2024 07:20
@ikalco ikalco mentioned this pull request Dec 22, 2024
Open
1 task
@ikalco ikalco force-pushed the improve_monitor_rule_mode_selection branch from e997038 to d922726 Compare December 22, 2024 19:08
@ikalco ikalco marked this pull request as ready for review December 22, 2024 19:17
@ikalco
Copy link
Contributor Author

ikalco commented Dec 22, 2024 •
edited
Loading

this is ready for review
just need figure out how to test custom modes and do that

edit:
highrr is broken cause of sorting stuff
will get to it later
so no review needed yet

@ikalco ikalco force-pushed the improve_monitor_rule_mode_selection branch from 8c14725 to 7030c98 Compare December 23, 2024 02:29
@ikalco
Copy link
Contributor Author

ikalco commented Dec 23, 2024 •
edited
Loading

ready for review

rebased + fixed
tested highrr, highres, requested mode, custom mode, and all work for me ™️
also fallbacks are working

@ikalco ikalco changed the title refactor/improve monitor mode selection (WIP) refactor/improve monitor mode selection Dec 23, 2024
@vaxerski
Copy link
Member

ping me after xmas

@ikalco
Copy link
Contributor Author

ikalco commented Dec 27, 2024 •
edited
Loading

@vaxerski its after xmas :)

vaxerski
vaxerski reviewed Dec 27, 2024
View reviewed changes
Copy link
Member

@vaxerski vaxerski left a comment

Choose a reason for hiding this comment

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

nice work in general. I like the approach.

src/helpers/Monitor.cpp Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vaxerski vaxerski vaxerski left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
config core helpers managers render
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ikalco @vaxerski