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

Refactor settings and related code #2660

Merged
merged 28 commits into from
Jan 1, 2025
Merged

Conversation

MattHag
Copy link
Collaborator

@MattHag MattHag commented Nov 5, 2024

Refactor NamedInts to native Flags and IntEnums to drastically enhance readability and simplify the code. Explicitly using enum members in favor of error-prone string comparisons will reduce future errors as well.

Related #2273 #2659

@MattHag MattHag requested a review from pfps November 5, 2024 09:07
@MattHag MattHag changed the title Refactor settings Refactor settings and related code Nov 5, 2024
@MattHag MattHag marked this pull request as draft November 5, 2024 09:12
@MattHag MattHag marked this pull request as ready for review November 6, 2024 08:54
@MattHag
Copy link
Collaborator Author

MattHag commented Nov 6, 2024

Please review and merge before any other change.

@MattHag
Copy link
Collaborator Author

MattHag commented Nov 8, 2024

@pfps Please have a look at this one, then I'll push through the remaining pieces of NamedInt.

Are we in the stabilization phase for a release?

@MattHag MattHag force-pushed the refactor_settings branch 2 times, most recently from da865af to 46bdc38 Compare November 16, 2024 14:58
@rloutrel
Copy link
Contributor

rloutrel commented Nov 19, 2024

@MattHag :
I wanted to do hidpp10_constants.INFO_SUBREGISTERS. I do not want to conflict with what you are doing.
1/ Are you on it?
2/ Is the reason of your use of Flag instead of IntEnum the size of the values?
3/ To avoid conflicts, I will either wait for this merge to be finished or start from the source version. What do you think is smarter?

@MattHag
Copy link
Collaborator Author

MattHag commented Nov 19, 2024

No, I have pushed everything. I use flags whenever the values where used as flags and are single bits, that can be combined together. You get the NamedInts functionality that was implemented in a more complicated way for free.

Just start from this branch right now and just wait for it to be merged, before ou create the pulls request. Then you don't have any issues and don't loose time.

@rloutrel
Copy link
Contributor

I created the PR on the originator branch of your fork, if this is what you meant.

@pfps
Copy link
Collaborator

pfps commented Dec 1, 2024

Check ordering of various settings.

@MattHag
Copy link
Collaborator Author

MattHag commented Dec 1, 2024

Fixed Kind enum and pushed it. I don't see anything different on MX Master and MX Keys, nor am I aware of changes.

If it's nowhere explicitly enforced, it shouldn't be a huge deal to add explicit ordering on top of these changes for specific entries.

@MattHag MattHag force-pushed the refactor_settings branch 3 times, most recently from 840d4df to 17482b3 Compare December 24, 2024 13:00
@MattHag
Copy link
Collaborator Author

MattHag commented Dec 24, 2024

@pfps Can we have an experimental branch where features are continually merged and don't pile up due to stabilization phase of a release? As long as tests run and nothing is obviously broken, it should be merged, when tests pass and review says it's fine. Thus, we can get to a test pipelin that covers all relevant pieces of code much faster than now. Nd people might wanna help, when they see progress.

@pfps
Copy link
Collaborator

pfps commented Dec 24, 2024

I just fixed a problem with the conversion to IntEnum. The problem is that the previous code could dynamically add named integers, which then acted just like the ones available at startup. But this is not possible with Enums so strings are used instead. But the strings then have to be handled specially. I fixed solaar show in preparation for release 1.1.14, but all the changes need to be investigated to determine whether Enums are suitable where they are used in this PR.

Copy link
Collaborator

@pfps pfps left a comment

Choose a reason for hiding this comment

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

Needs checking to ensure that any strings used to extend the enums are handled correctly.

Copy link
Collaborator

@pfps pfps left a comment

Choose a reason for hiding this comment

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

This bug needs to be fixed.

*** output flushed ***
idefix Solaar> git checkout pull_2660
Already on 'pull_2660'
idefix Solaar> bin/solaar show
solaar version 1.1.14rc4-28-g2be15c9b


MX Master 3 Wireless Mouse
     Device path  : /dev/hidraw1
     USB id       : 046d:B023
     Codename     : MX Master 3
     Kind         : mouse
     Protocol     : HID++ 4.5
     Serial number: 
     Model ID:      B02340820000
     Unit ID:       198E3EB8
                 1: BOT 95.01.B0015
                 0: MPM 19.01.B0015
                 3: 
     Supports 31 HID++ 2.0 features:
         0: ROOT                   {0000} V0     
         1: FEATURE SET            {0001} V0     
         2: DEVICE FW VERSION      {0003} V3     
            Firmware: 1 BOT 95.01.B0015 00006A9CA89D
            Firmware: 0 MPM 19.01.B0015 B0236A9CA89D
            Firmware: 3   
            Unit ID: 198E3EB8  Model ID: B02340820000  Transport IDs: {'btleid': 'B023', 'wpid': '4082'}
         3: DEVICE NAME            {0005} V0     
            Name: Wireless Mouse MX Master 3
            Kind: mouse
         4: WIRELESS DEVICE STATUS {1D4B} V0     
         5: CONFIG CHANGE          {0020} V0     
            Configuration: 11000000000000000000000000000000
         6: CRYPTO ID              {0021} V1     
         7: DEVICE FRIENDLY NAME   {0007} V0     
            Friendly Name: MX Master 3
         8: BATTERY STATUS         {1000} V1     
            Battery: 100%, BatteryStatus.DISCHARGING, next level 50%.
         9: REPROG CONTROLS V4     {1B04} V4     
            Key/Button Actions (saved): {Left Button:Left Click, Right Button:Right Click, Middle Button:Mouse Middle Button, Back Button:Mouse Back Button, Forward Button:Mouse Forward Button, Mouse Gesture Button:Gesture Button Navigation, Smart Shift:Smart Shift}
            Key/Button Actions        : {Left Button:Left Click, Right Button:Right Click, Middle Button:Mouse Middle Button, Back Button:Mouse Back Button, Forward Button:Mouse Forward Button, Mouse Gesture Button:Gesture Button Navigation, Smart Shift:Smart Shift}
            Key/Button Diversion (saved): {Middle Button:Regular, Back Button:Regular, Forward Button:Regular, Mouse Gesture Button:Regular, Smart Shift:Regular}
            Key/Button Diversion        : {Middle Button:Regular, Back Button:Regular, Forward Button:Regular, Mouse Gesture Button:Regular, Smart Shift:Regular}
        10: CHANGE HOST            {1814} V1     
            Change Host        : 3:idefix
        11: XY STATS               {2250} V1     
        12: ADJUSTABLE DPI         {2201} V1     
            Sensitivity (DPI) (saved): 1000
            Sensitivity (DPI)        : 1000
        13: SMART SHIFT            {2110} V0     
            Scroll Wheel Ratcheted (saved): Ratcheted
            Scroll Wheel Ratcheted        : Ratcheted
            Scroll Wheel Ratchet Speed (saved): 23
            Scroll Wheel Ratchet Speed        : 23
        14: HIRES WHEEL            {2121} V1     
            Multiplier: 15
            Has invert: Normal wheel motion
            Has ratchet switch: Normal wheel mode
            High resolution mode
            HID notification
            Scroll Wheel Direction (saved): False
            Scroll Wheel Direction        : False
            Scroll Wheel Resolution (saved): True
            Scroll Wheel Resolution        : True
            Scroll Wheel Diversion (saved): False
            Scroll Wheel Diversion        : False
        15: THUMB WHEEL            {2150} V0     
            Thumb Wheel Direction (saved): False
            Thumb Wheel Direction        : False
            Thumb Wheel Diversion (saved): False
            Thumb Wheel Diversion        : False
        16: WHEEL STATS            {2251} V0     
        17: DFUCONTROL SIGNED      {00C2} V0     
        18: DEVICE RESET           {1802} V0    internal, hidden 
        19: unknown:1803           {1803} V0    internal, hidden 
        20: CONFIG DEVICE PROPS    {1806} V6    internal, hidden 
        21: unknown:1813           {1813} V0    internal, hidden 
        22: OOBSTATE               {1805} V0    internal, hidden 
        23: unknown:1830           {1830} V0    internal, hidden 
        24: unknown:18A1           {18A1} V0    internal, hidden 
        25: unknown:1E00           {1E00} V0    hidden 
        26: unknown:1EB0           {1EB0} V0    internal, hidden 
        27: unknown:1861           {1861} V0    internal, hidden 
        28: unknown:9300           {9300} V0    internal, hidden 
        29: unknown:9001           {9001} V0    internal, hidden 
        30: unknown:9205           {9205} V0    internal, hidden 
     Has 8 reprogrammable keys:
         0: Left Button               , default: Left Click                  => Left Click                
solaar: error: Traceback (most recent call last):
  File "/home/local/SoftwareDownloads/Solaar/lib/solaar/cli/__init__.py", line 221, in run
    m.run(c, args, _find_receiver, _find_device)
    ~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/local/SoftwareDownloads/Solaar/lib/solaar/cli/show.py", line 332, in run
    _print_device(d)
    ~~~~~~~~~~~~~^^^
  File "/home/local/SoftwareDownloads/Solaar/lib/solaar/cli/show.py", line 280, in _print_device
    print(f"             {', '.join(k.flags)}, pos:{int(k.pos)}, group:{int(k.group):1}, group mask:{gmask_fmt}")
                          ~~~~~~~~~^^^^^^^^^
TypeError: sequence item 0: expected str instance, KeyFlag found

@MattHag
Copy link
Collaborator Author

MattHag commented Dec 24, 2024

There's probably a property missing. Will have a look.

- Move validators into their own module.
- Convert Kind to IntEnum

Related pwr-Solaar#2273
MattHag and others added 24 commits January 1, 2025 16:04
Classes shouldn't don't need to know about other settings classes.

Related pwr-Solaar#2273
Enforce create_widgets and collect_values.

Related pwr-Solaar#2273
Refactor code related to task and task ID.

Related pwr-Solaar#2273
The key flags are solely used in hiddpp20 module, thus put them into the
module.

Related pwr-Solaar#2273
This data is not in use currently.

Related pwr-Solaar#2273
The mapping flags are solely used in hiddpp20 module, thus put them into
this module.

Related pwr-Solaar#2273
The charge status is solely used in the hiddpp20 module, thus put it
into this module.

Related pwr-Solaar#2273
Ensure behavior stays the same.

Related pwr-Solaar#2273
Copy link
Collaborator

@pfps pfps left a comment

Choose a reason for hiding this comment

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

OK, no problems seen now.

@pfps pfps merged commit 96364d2 into pwr-Solaar:master Jan 1, 2025
5 checks passed
@pfps
Copy link
Collaborator

pfps commented Jan 1, 2025

What PR should I look at next?

@MattHag
Copy link
Collaborator Author

MattHag commented Jan 1, 2025

The macOS Bluetooth one and the then start from the latest ones. They are quite independent and small. I can fix the older PR then, there's no other huge one.

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