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

Changes to --list and more #52

Merged
merged 8 commits into from
Feb 9, 2021
Merged

Changes to --list and more #52

merged 8 commits into from
Feb 9, 2021

Conversation

LoLei
Copy link
Owner

@LoLei LoLei commented Feb 2, 2021

Regarding #51

@GM-Script-Writer-62850
Copy link

so as for the man pages for list_devices_* 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
Copy link

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
Copy link

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
Copy link

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
Copy link
Owner Author

LoLei commented Feb 3, 2021

so as for the man pages for list_devices_* 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
Copy link
Owner Author

LoLei commented Feb 3, 2021

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
Copy link

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
Copy link
Owner Author

LoLei commented Feb 3, 2021

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
Copy link

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
Copy link
Owner Author

LoLei commented Feb 3, 2021

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
Copy link

GM-Script-Writer-62850 commented Feb 3, 2021

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
Copy link

GM-Script-Writer-62850 commented Feb 3, 2021

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
Copy link

GM-Script-Writer-62850 commented Feb 4, 2021

changes.patch.zip

  • add support to use x color as a color value
  • force -a to use xcolor
    • should -a set brightness?
  • eliminate need to import sys
    • edit: broke using -man to list all options by doing that
  • drop man pages for list_devices_long and list_devices_short
  • add man page for -a

@GM-Script-Writer-62850
Copy link

GM-Script-Writer-62850 commented Feb 4, 2021

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
Copy link
Owner Author

LoLei commented Feb 6, 2021

These latest changes work for me.

@GM-Script-Writer-62850
Copy link

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

  • this make --restore prefer using a devices serial number (if someone has multiple keyboards or mouses of the same model)
  • tried to fix possible issues with old setting files

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

  • remove brightness as a effect (this will result in a error message saying brightness is not supported by the device if you try to use it as a effect, when that not entirely true)
  • store the time the setting was set and use that to decide what to use

@LoLei
Copy link
Owner Author

LoLei commented Feb 7, 2021

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
Copy link

Regarding the --restore option:

  • settings-brightness_superior.patch = -b > -e
  • settings-effect_brightness_superior.patch = -b < -e
    You can set brightness 2 ways, nigher of those patches solve nor work around it, they just let you pick what one get propriety for the --restore feature

@GM-Script-Writer-62850
Copy link

GM-Script-Writer-62850 commented Feb 7, 2021

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
Copy link
Owner Author

LoLei commented Feb 7, 2021

* 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
Copy link

GM-Script-Writer-62850 commented Feb 7, 2021

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
Copy link

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
Copy link
Owner Author

LoLei commented Feb 7, 2021

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
Copy link

GM-Script-Writer-62850 commented Feb 7, 2021

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
Copy link
Owner Author

LoLei commented Feb 8, 2021

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
Copy link
Owner Author

LoLei commented Feb 8, 2021

@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
Copy link

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
Copy link
Owner Author

LoLei commented Feb 9, 2021

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

I'd say we merge it.

@LoLei LoLei merged commit f1dd191 into master Feb 9, 2021
@LoLei LoLei deleted the feature/list-changes branch February 9, 2021 19:28
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.

None yet

2 participants