search

LoLei / razer-cli

CLI for configuring Razer devices
GNU General Public License v3.0
93 stars 11 forks source link

Changes to --list and more #52

Closed LoLei closed 3 years ago

LoLei commented 3 years ago

Regarding #51

GM-Script-Writer-62850 commented 3 years ago

so as for the man pages for listdevices* i made those a symlink to list_devices is that not something we can do here? if not we may as well drop the alias as there is no point in maintaining 3 copies of the same file

GM-Script-Writer-62850 commented 3 years ago

Does flake8 complain about this?

--- razer_cli/razer_cli.py  2021-02-02 13:20:13.429779654 -0500
+++ razer_cli-new/razer_cli.py  2021-02-02 13:44:24.602185000 -0500
@@ -170,8 +170,8 @@
                         print("      brightness: N/A")
                     try:
                         # hasattr will error anyway; no point in testing with it
-                        print("      colors:", util.bytes_array_to_hex_array(
-                            device.fx.colors))
+                        print("      colors:",
+                            util.bytes_array_to_hex_array(device.fx.colors))
                     except:
                         if args.verbose:
                             print("      colors: N/A")
@@ -815,8 +815,8 @@

     if args.version:
         print("razer-cli:", __version__)
-        print("   Installed in", util.os.path.dirname(
-            util.os.path.realpath(__file__)))
+        print("   Installed in",
+            util.os.path.dirname(util.os.path.realpath(__file__)))
         print("python3-openrazer:", device_manager.version)
         print("openrazer-daemon:", device_manager.daemon_version)
GM-Script-Writer-62850 commented 3 years ago

you said you liked commas in the -l option so for the sake of consistency

--- razer_cli/util.py   2021-02-02 13:19:50.029988454 -0500
+++ razer_cli_dev/util.py   2021-02-02 14:05:03.554643181 -0500
@@ -159,7 +159,7 @@
 def print_manual(man):
     d_path = os.path.dirname(os.path.realpath(__file__))+'/man_pages'
     if len(man) == 0:
-        return print("Manual entries exist for:", *sorted(os.listdir(d_path)))
+        return print("Manual entries exist for:", ', '.join(sorted(os.listdir(d_path))))
     for i in man:
         f_path = d_path+'/'+i
         if os.path.isfile(f_path):
@@ -168,4 +168,4 @@
                 print(f.read())
         else:
             print("No manual entries exist for", i,
-                  "try:\n  ", *sorted(os.listdir(d_path)))
+                  "try:\n  ", ', '.join(sorted(os.listdir(d_path))))
GM-Script-Writer-62850 commented 3 years ago

Fix copy/paste mistake:

--- razer_cli/razer_cli.py  2021-02-02 13:20:13.429779654 -0500
+++ razer_cli_dev/razer_cli.py  2021-02-02 21:24:24.507494824 -0500
@@ -124,7 +124,7 @@
     if args.verbose and args.device:
         print("   Only showing devices matching:", args.device)

-    # Iterate over each device and set DPI
+    # Iterate over each device and pretty out some standard information about each
     for device in device_manager.devices:
         # If -d argument is set, only list those devices
         if (args.device and (device.name in args.device or device.serial in args.device)) or (not args.device):
LoLei commented 3 years ago

so as for the man pages for listdevices* i made those a symlink to list_devices is that not something we can do here? if not we may as well drop the alias as there is no point in maintaining 3 copies of the same file

I did not receive any symlink files in the zip archive. We can just leave them as copies, which is easier to track on git anyway.

LoLei commented 3 years ago

Does flake8 complain about this?

--- razer_cli/razer_cli.py    2021-02-02 13:20:13.429779654 -0500
+++ razer_cli-new/razer_cli.py    2021-02-02 13:44:24.602185000 -0500
@@ -170,8 +170,8 @@
                         print("      brightness: N/A")
                     try:
                         # hasattr will error anyway; no point in testing with it
-                        print("      colors:", util.bytes_array_to_hex_array(
-                            device.fx.colors))
+                        print("      colors:",
+                            util.bytes_array_to_hex_array(device.fx.colors))
                     except:
                         if args.verbose:
                             print("      colors: N/A")
@@ -815,8 +815,8 @@

     if args.version:
         print("razer-cli:", __version__)
-        print("   Installed in", util.os.path.dirname(
-            util.os.path.realpath(__file__)))
+        print("   Installed in",
+            util.os.path.dirname(util.os.path.realpath(__file__)))
         print("python3-openrazer:", device_manager.version)
         print("openrazer-daemon:", device_manager.daemon_version)

It does indeed complain, as it violates PEP8:

E128 continuation line under-indented for visual indent

GM-Script-Writer-62850 commented 3 years ago

guess it just followed the links

$ ls -la
total 48
drwxrwxr-x 2 chad chad 4096 Feb  2 13:45 .
drwxrwxr-x 3 chad chad 4096 Feb  2 13:45 ..
-rw-rw-r-- 1 chad chad  293 Feb  1 10:53 battery
-rw-rw-r-- 1 chad chad  466 Feb  1 10:50 brightness
-rw-rw-r-- 1 chad chad  454 Feb  1 10:30 color
-rw-rw-r-- 1 chad chad  173 Feb  1 11:14 device
-rw-rw-r-- 1 chad chad  249 Feb  1 10:27 dpi
-rw-rw-r-- 1 chad chad 1881 Feb  2 10:44 effect
-rw-rw-r-- 1 chad chad 2537 Feb  2 11:18 example
-rw-rw-r-- 1 chad chad  306 Feb  1 10:34 list_devices
lrwxrwxrwx 1 chad chad   12 Feb  2 13:45 list_devices_long -> list_devices
lrwxrwxrwx 1 chad chad   12 Feb  2 13:45 list_devices_short -> list_devices
-rw-rw-r-- 1 chad chad  166 Feb  1 10:31 poll
-rw-rw-r-- 1 chad chad  685 Feb  1 10:30 zone
LoLei commented 3 years ago

I just noticed that the changes in this pull request break the -a flag. That flag is the main reason I created and use this tool. Could you please fix it? It is supposed to get the colors from the X resources via the get_x_color function, which I see is still used in the code but apparently something prevents the flag from correctly applying the colors.

Reference output on master (Settings to red first and then setting to automatic afterwards, which works):

me@keyboard ~
$ razer-cli -v -c ff0000
RGB:
    [255, 0, 0]
    If more are needed random ones will be generated
Setting effects for Razer Lancehead Tournament Edition:
    generic does not support static
    Setting logo static to: 255 0 0
    Setting scroll_wheel static to: 255 0 0
    Setting left static to: 255 0 0
    Setting right static to: 255 0 0
    Device does not support backlight
Setting effects for Razer BlackWidow X Chroma Tournament Edition:
    Setting generic static to: 255 0 0
    Device does not support logo
    Device does not support scroll_wheel
    Device does not support left
    Device does not support right
    Device does not support backlight

me@keyboard ~
$ razer-cli -v -a
RGB:
    [167, 78, 52]
    If more are needed random ones will be generated
Setting effects for Razer Lancehead Tournament Edition:
    generic does not support static
    Setting logo static to: 167 78 52
    Setting scroll_wheel static to: 167 78 52
    Setting left static to: 167 78 52
    Setting right static to: 167 78 52
    Device does not support backlight
Setting effects for Razer BlackWidow X Chroma Tournament Edition:
    Setting generic static to: 167 78 52
    Device does not support logo
    Device does not support scroll_wheel
    Device does not support left
    Device does not support right
    Device does not support backlight

Output on this branch (Settings to red first and then setting to automatic afterwards, which does not work and red remains):

me@keyboard ~
$ razer-cli -v -c ff0000
Setting effects for Razer Lancehead Tournament Edition:
   generic:
      Does not support static
   logo:
      Setting static to: 255 0 0
   scroll_wheel:
      Setting static to: 255 0 0
   left:
      Setting static to: 255 0 0
   right:
      Setting static to: 255 0 0
   backlight:
      Device does not support backlight
Setting effects for Razer BlackWidow X Chroma Tournament Edition:
   generic:
      Setting static to: 255 0 0
   logo:
      Device does not support logo
   scroll_wheel:
      Device does not support scroll_wheel
   left:
      Device does not support left
   right:
      Device does not support right
   backlight:
      Device does not support backlight

me@keyboard ~
$ razer-cli -v -a
Setting effects for Razer Lancehead Tournament Edition:
   generic:
      Does not support static
   logo:
      No color given, using: [[255, 0, 0], [0, 255, 0], [0, 0, 255]]
      Setting static to: 255 0 0
   scroll_wheel:
      No color given, using: [[255, 0, 0], [0, 255, 0], [0, 0, 255]]
      Setting static to: 255 0 0
   left:
      No color given, using: [[255, 0, 0], [0, 255, 0], [0, 0, 255]]
      Setting static to: 255 0 0
   right:
      No color given, using: [[255, 0, 0], [0, 255, 0], [0, 0, 255]]
      Setting static to: 255 0 0
   backlight:
      Device does not support backlight
Setting effects for Razer BlackWidow X Chroma Tournament Edition:
   generic:
      No color given, using: [[255, 0, 0], [0, 255, 0], [0, 0, 255]]
      Setting static to: 255 0 0
   logo:
      Device does not support logo
   scroll_wheel:
      Device does not support scroll_wheel
   left:
      Device does not support left
   right:
      Device does not support right
   backlight:
      Device does not support backlight
GM-Script-Writer-62850 commented 3 years ago

What is is doing it pulling the existing color from the mouse via, see man page for color, line 8 - 11 https://github.com/LoLei/razer-cli/blob/feature/list-changes/razer_cli/man_pages/color#L7-L11 What you found is a feature, not a bug, i see the your issue would a razer-cli -c x work for you instead of calling -a you could call razer-cli -c x

LoLei commented 3 years ago

I understand what the man page for -c is saying, but I'm not calling -c without a color, I'm calling -a, which is a different flag with a different behavior.

I'd like the -a flag to keep this existing behavior, as some scripts [1], [2] that use this for auto-coloring in combination with e.g. pywal use this flag, and I wouldn't like to to break backwards compatibility, for people who upgrade their python libs but want to continue using the same scripts.

GM-Script-Writer-62850 commented 3 years ago

Here are some options Create option to for X color as feature for -c

--- razer_cli/razer_cli.py  2021-02-02 13:20:13.429779654 -0500
+++ razer_cli_dev/razer_cli.py  2021-02-03 13:30:04.039218389 -0500
@@ -62,8 +61,8 @@
         stop = len(color)
         i = 0
         while i < stop:
-            if len(color[i]) > 3:
-                if not len(color[i]) == 6:
+            if len(color[i]) > 3 or color[i] in ['x', 'X'] :
+                if not len(color[i]) in [1, 6]:
                     print('color', len(RGB)+1,
                           '(', color[i], ') looks to have a typo')
                 RGB.append(parse_color_argument([color[i]]))
@@ -93,9 +92,11 @@

     if len(color) == 1:
         # Hex: Just one input argument
-        rgb = color[0]
-        if rgb.lower() == "random":
+        rgb = color[0].lower()
+        if rgb == "random":
             rgb = util.get_random_color_rgb()
+        elif rgb == "x":
+            rgb = get_x_color()
         else:
             rgb = util.hex_to_decimal(rgb)
     else:

Use X color for -a

--- razer_cli/razer_cli.py  2021-02-02 13:20:13.429779654 -0500
+++ razer_cli_dev/razer_cli.py  2021-02-03 13:30:04.039218389 -0500
@@ -758,6 +759,8 @@
         color = []
         if args.color:
             color = parse_color(args.color)
+        elif args.automatic:
+            color = [get_x_color()]
         zones = parse_zones(args.zones)
         if not args.effect:
             effects = ['static']
GM-Script-Writer-62850 commented 3 years ago

I'll let you update the color manual to mention that X color will be used if you use -a (X color returns a RNG color if no x_color is found) also -c will override -a

GM-Script-Writer-62850 commented 3 years ago

changes.patch.zip

GM-Script-Writer-62850 commented 3 years ago

revised patch changes.patch.zip also fixed some setting stuff and added battery settings

using -a will set brightness to 100% (if you do not define a effect, brightness, or set multiple zones (logo,wheel = 1 zone & logo wheel = 2 zones)

LoLei commented 3 years ago

These latest changes work for me.

GM-Script-Writer-62850 commented 3 years ago

If you have a settings file that causes a key error or anything i can get a copy of it? settings_fixes.zip

this has 2 patches, each handle --restore differently This feature has a bug when it has to decide if it should restore -e brightness or -b brightness, there is one patch for each option there are a few options to solve this issue

LoLei commented 3 years ago

I don't have a copy of my old settings file.

Which patch does what? I can't tell. They're called:

settings-brightness_superior.patch
settings-effect_brightness_superior.patch # I assume this is the one that removes brightness as an effect?

I can't tell the diff contents apart either, as they include partly the same changes.

GM-Script-Writer-62850 commented 3 years ago

Regarding the --restore option:

GM-Script-Writer-62850 commented 3 years ago

x_color_2_util.patch.zip here is another small patch moved get_x_color to the util.py file it now only gets the x_color once per session for better performance (in cause you use -c x FF00FF x) before this patch if there was no x color x could return blue the 1st time and red the next time, now if it returns green all it will be green til the next time the script runs

LoLei commented 3 years ago 
* settings-brightness_superior.patch = `-b` > `-e`
* settings-effect_brightness_superior.patch = `-b` < `-e`

What does that mean?

x_color_2_util.patch.zip here is another small patch moved get_x_color to the util.py file it now only gets the x_color once per session for better performance (in cause you use -c x FF00FF x) before this patch if there was no x color x could return blue the 1st time and red the next time, now if it returns green all it will be green til the next time the script runs

Is this patch based on the previous two you uploaded or does it include the changes of one? Or is it completely independent?

Please just give me one patch that is based on the latest changes in this PR, that includes everything you still want to add. I don't care at this point which of the brightness options or whatever is used, I just want this to be merged.

If it makes any difference, I'd prefer it if -b keeps existing as a flag.

GM-Script-Writer-62850 commented 3 years ago

that last one is independent (applied after either of the last 2)

settings-brightness_superior.patch and settings-effect_brightness_superior.patch only affect the --restore option

lets say you did this:

$ razer-cli -a
$ razer-cli -b 50

settings-brightness_superior.patch --restore will give this: razer-cli -d 321848H04401822 -c 102 180 75 -z generic,logo,scroll_wheel,left,right,backlight generic,logo,scroll_wheel,left,right,backlight -e static brightness -b generic 50 logo 50 scroll_wheel 50 left 50 right 50 backlight 50 In this command brightness will be set to 100% then get set to 50% later in the same command as -b is processed after -e

settings-effect_brightness_superior.patch --restore will give this: razer-cli -d 321848H04401822 -c 102 180 75 -z generic,logo,scroll_wheel,left,right,backlight generic,logo,scroll_wheel,left,right,backlight -e static brightness In this command -b was omitted as -e has brightness set to apply to every zone

As it currently is --restore has no way to know the order you ran your 2 commands as and since you can apply brightness in 2 methods therein lies the issue

the settings-effect_brightness_superior.patch moves -b from each lighting zone generated by --restore where brightness is set as a effect of said zone

NO patch has removed -b nor brightness from effects

GM-Script-Writer-62850 commented 3 years ago

Personally i think the current implementation of save/load settings is quite poor, admittedly it is better than when i started messing with it, i think the entire structure of the settings file should be changed to allow a proper was to keep track of what was set to what zone, the current save was not designed to keep track of stacked commands each targeting different zones of the hardware too bad we just can't read all device settings on a --save flag and dump that to a file for --load flag as long as we want to support the current stable release of openrazer

LoLei commented 3 years ago

Ah that makes sense to me now, thanks. I falsely assumed the -b flag might be removed in some way or another. I have no strong feelings one way or the other then.

If you wanna give the whole settings functionality an overhaul feel free to do so, but let's merge this PR first. I'll add your latest changes to it tomorrow, probably, as I don't have the code repository with me right now.

GM-Script-Writer-62850 commented 3 years ago

no plans of stating that today, maybe next week or when ever i feel like it, but if i do i am creating a new setting file(s) and it will not touch the old one it can sit in the folder till the user deletes it manually incase they want to read it Removing -b would make commands a pita as you would need to include each zone 2 times to set brightness and a effect that would not be user frendly at all, letting someone do that if they feel like it is fine, but forcing that on a user...

LoLei commented 3 years ago

I couldn't apply the last patch you uploaded here, because something changed after applying the previous patches, I assume.

$ git apply ~/razer_cli_patch/15/x_color_2_util.patch
error: patch failed: razer_cli/util.py:108
error: razer_cli/util.py: patch does not apply

So I manually copied each change into the code. This is getting unbearably tedious. After this pull request I will most likely not accept any more changes from contributors that do not come in a pull request directly from them. I appreciate every contribution, but dealing with them like this is not worth it.

LoLei commented 3 years ago

@GM-Script-Writer-62850 Please look through the changes of the pull request once more and test everything yourself, if everything is working we can merge it.

GM-Script-Writer-62850 commented 3 years ago

I did notice one other very minor change we can make both util.py and razer_cli.py import settings wouldn't there be a performance uplift if we do not import settings in razer_cli.py and instead put settings = util.settings after util.py is imported?

LoLei commented 3 years ago

That seems like such a minuscule performance improvement, I wouldn't bother.

I'd say we merge it.