owenthewizard / i3lock-next

Create a fancy image to use with i3lock.
MIT License
63 stars 9 forks source link

Lock image size is hard-coded and square #13

Closed kylecarbon closed 6 years ago

kylecarbon commented 6 years ago

Just wanted to start by saying thanks for i3lock-next! Definitely the fastest locker I've come across so far.

I was wondering if it'd be possible to make i3lock-next a little more flexible with regards to the lock image, specifically:

  1. lookup lock image size directly in i3lock-next-helper.c
  2. potentially adjust the ring size argument that's passed to i3lock-color

I've done step 1 already on my fork and can submit a PR if you're agreeable. Otherwise, I'm curious as to what your thoughts are.

owenthewizard commented 6 years ago

I appreciate your kind words! I would love to implement both features, go ahead and submit a PR.

owenthewizard commented 6 years ago

I rewrote the i3lock-next script in Python on the kcarbon-lookup_lock_image_size branch, let me know what you think.

kylecarbon commented 6 years ago

I'm personally happier with scripting in Python and your branch/script looks good! I'm about to get on a plane though, so I didn't have a chance to test it out. This is where my ignorance comes up (aerospace background, not software), but how much slower is the Python script versus the bash script? I'd assume not enough to become perceptible to the user?

Also per your comment on the PR, I did use bc at first but couldn't get the float back to an int. I didn't see a floor function listed or anything and didn't want to write yet another helper function in bash.

owenthewizard commented 6 years ago

In my expert (read: definitely NOT expert) opinion, Python should be faster than Bash. Getting the image size with Python instead of with C could be slower but in my (very unscientific) testing it didn't seem slower.

Edit: My statement that Python is faster than Bash is just a personal rule-of-thumb and afaik not backed up by any testing.

ghost commented 6 years ago

Got a chance to test your new branch! Comments:

owenthewizard commented 6 years ago
  1. I'll look into this today.
    • rstrip removes trailing whitespace, in this case the newline.
    • Duly noted.
  2. Yes, but I think making this configurable via a config file would be good.
  3. I'll fix this today as well.
  4. :smiley:
owenthewizard commented 6 years ago

Okay, I took the the code that prints color out of the loop (d'oh!), added a config for the clock position and some other options, and fixed the radius calculation. If you could test the latest commit that would be great!

kylecarbon commented 6 years ago

Hey! Sorry, I'm not on holiday anymore and am usually pretty busy during the week. I will definitely get to this by this upcoming Sunday but hopefully sooner.

owenthewizard commented 6 years ago

No problem, I appreciate your help.

kylecarbon commented 6 years ago

Looking pretty sweet! I did run into a couple things

  1. Some of the clock positions weren't working.
    1. working: bottom-right, top-right, top-left
    2. not working: bottom-center, bottom-left, top-center (it's at the top of my middle monitor, about 1/4 of the way over from the left)
  2. Issues with clock colour:
    1. clock colour is always based on colour of center monitor, regardless of where you actually put the clock.
    2. the logic for detecting the center monitor's colour isn't quite right? Eg.) my center monitor is dark yet it's outputting a dark colour for the font, so it's hard to see the time.
    3. maybe this is why I just couldn't see the clock...
  3. i3lock-next.ini is located in ~/.config. Should it maybe go in ~/.config/i3lock-next?
  4. Sneaky feature request: add image path in the .ini file :D it'd be cool to be able to swap out images whenever you want without having to re-build