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

Light up buttons #25

Merged
merged 23 commits into from
Apr 24, 2024
Merged

Light up buttons #25

merged 23 commits into from
Apr 24, 2024

Conversation

Jokilos
Copy link
Contributor

@Jokilos Jokilos commented Mar 13, 2024

Allows user to light up the elements of the controller, when setting the binds.

Copy link

codecov bot commented Mar 13, 2024

Codecov Report

Attention: Patch coverage is 7.69231% with 96 lines in your changes are missing coverage. Please review.

Project coverage is 41.82%. Comparing base (a3c64f6) to head (556c9bc).
Report is 10 commits behind head on main.

❗ Current head 556c9bc differs from pull request most recent head d7b1f63. Consider uploading reports for the commit d7b1f63 to get more accurate results

Files Patch % Lines
midi_app_controller/gui/binds_editor.py 0.00% 64 Missing ⚠️
..._app_controller/controller/connected_controller.py 21.05% 30 Missing ⚠️
midi_app_controller/gui/midi_status.py 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #25      +/-   ##
==========================================
- Coverage   46.09%   41.82%   -4.27%     
==========================================
  Files          12       12              
  Lines         640      722      +82     
==========================================
+ Hits          295      302       +7     
- Misses        345      420      +75     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

midi_app_controller/controller/connected_controller.py Outdated Show resolved Hide resolved
midi_app_controller/controller/connected_controller.py Outdated Show resolved Hide resolved
midi_app_controller/gui/midi_status.py Outdated Show resolved Hide resolved
midi_app_controller/controller/connected_controller.py Outdated Show resolved Hide resolved
midi_app_controller/gui/binds_editor.py Outdated Show resolved Hide resolved
midi_app_controller/gui/binds_editor.py Outdated Show resolved Hide resolved
midi_app_controller/gui/binds_editor.py Outdated Show resolved Hide resolved
midi_app_controller/gui/binds_editor.py Outdated Show resolved Hide resolved
midi_app_controller/gui/binds_editor.py Outdated Show resolved Hide resolved
midi_app_controller/controller/connected_controller.py Outdated Show resolved Hide resolved
midi_app_controller/gui/binds_editor.py Show resolved Hide resolved
midi_app_controller/gui/binds_editor.py Outdated Show resolved Hide resolved
midi_app_controller/gui/binds_editor.py Outdated Show resolved Hide resolved
midi_app_controller/gui/binds_editor.py Outdated Show resolved Hide resolved
midi_app_controller/gui/binds_editor.py Outdated Show resolved Hide resolved
midi_app_controller/gui/binds_editor.py Outdated Show resolved Hide resolved
midi_app_controller/controller/connected_controller.py Outdated Show resolved Hide resolved
midi_app_controller/controller/connected_controller.py Outdated Show resolved Hide resolved
midi_app_controller/gui/binds_editor.py Outdated Show resolved Hide resolved
@@ -197,6 +197,7 @@ def save(knob_binds: List[KnobBind], button_binds: List[ButtonBind]) -> None:
# TODO Get actions directly from app-model when it's supported.
state_manager.actions,
save,
state_manager._connected_controller,
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe remove the _ prefix from the _connected_controller in ConnectedController since it's used outside now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you elaborate on this comment? I don't quite get what should I change.

Copy link
Contributor

@lukaszsmolinski lukaszsmolinski Apr 1, 2024

Choose a reason for hiding this comment

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

in ConnectedController

Sorry, I meant StateManager instead of ConnectedController in my previous message.

_connected_controller has a leading _, which indicates that it should only be accessed inside StateManager. It's not the case anymore, so I suggests renaming it (simply by removing the _).

Copy link
Contributor

Choose a reason for hiding this comment

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

There are two more occurrences of _connected_controller in the code in case you didn't notice them. They don't fail tests because they aren't tested yet. @Jokilos

midi_app_controller/controller/connected_controller.py Outdated Show resolved Hide resolved
butons_mutex : QMutex
Mutex for worker threads.
flashing_buttons : set[int]
Set of the button ids, that are currently flashing.
Copy link
Contributor

Choose a reason for hiding this comment

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

slightly ungrammatical, maybe "Set with ids of buttons that are currently flashing"?

light_up_button.clicked.connect(lambda: self._light_up_button(button_id))

sizes = [2, 2, 6, 10]
elems = [
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe do elems = [ (2, button_label), ...] and later for elem, size in elems

@Jokilos Jokilos merged commit 1050d2e into main Apr 24, 2024
10 of 11 checks passed
@Jokilos Jokilos deleted the light-up-buttons branch April 24, 2024 20:54
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.

4 participants