lunaneff / gnome-shell-extension-zenbook-duo

GNU General Public License v3.0
14 stars 10 forks source link

Add file permission check & enable GNOME41 support #1

Closed jibsaramnim closed 2 years ago

jibsaramnim commented 2 years ago

First off; thank you so much for building this extension, it's been a joy to use my UX482 running Fedora and having both screens' brightness synchronize is quite an elegant solution, it almost makes you forget this was achieved with an extension and isn't just how it natively works. Well done! :)


As far as I could test with my Zenbook model, your extension seems to work perfectly with GNOME41 too, so this PR adds that version to the whitelist. I also wanted to see if I could add proper file permission checking support. Being new to gnome extension development, this part in particular would probably benefit from a double check on your part.

Lastly, I added basic .editorconfig and .prettierrc configuration files and tried to have them mostly match your existing coding standards of this project. A few line break changes were applied, but I hope they contribute to the code's readability and git log traversing.


I hope it's alright that I made this PR. I'm interested in seeing if we can find some useful functionality for some of the other buttons these Asus keyboards have available. I have never actually used Windows on this laptop, so I'm not sure what the "Tools" button originally did, but there might be some interesting possibilities here -- though these would likely benefit from having customizable settings added too. For example, the "swap windows" button might be extra useful if there was an extension setting that would allow you to choose to either swap all windows, or just the active one. Something like that.

Anyway, thank you again, and please let me know if you'd like me to make any changes to this PR.

lunaneff commented 2 years ago

Thanks for the PR! I can't properly test it right now, so I'll just comment on some other things for now:

Actually syncing the brightness wasn't even on Windows (there you have separate sliders), so that was an original idea. Nice that you like it this way

Cool to know that it works on models other than mine. A section in the readme that lists known working models would be nice to have, but I can add that myself

I'm actually new to extension development as well and don't know the best way to check permissions (that's why it wasn't implemented yet), so I'll probably just check if it works, and if it does, I'll accept it

I'm not sure what the "Tools" button originally did

On Windows it launches a proprietary configuration app called MyASUS, and since that doesn't exist on Linux, I thought of making it launch a user configurable app. But I like your idea as well, so I'll probably let the user choose between launching an app and some other options

jibsaramnim commented 2 years ago

A section in the readme that lists known working models would be nice to have, but I can add that myself

This would be useful indeed! I wasn't actually sure which model you have, but everything seems to be working exactly as you'd expect on my device too. I have to look up the exact model number, as there's a few variations of the UX482 I have, I can't remember it off the top of my head.

I'm actually new to extension development as well and don't know the best way to check permissions (that's why it wasn't implemented yet), so I'll probably just check if it works, and if it does, I'll accept it

Thank you! I'm glad we're both figuring it out as we go 😂. It took a bit of effort to wade through their documentation. It's certainly lacking examples to allow you to better understand how things actually work.

I am checking both read and write permissions, I figured it'll be wise to look for both, just to be absolutely certain.

A small note on a slight functional change: I wrapped the entire permission check inside an else to the "does the file exist" if. Originally it did both in a row, but I thought it'd be cleaner to not even bother with a permission check if the file doesn't actually exist.

so I'll probably let the user choose between launching an app and some other options

Nice! I think a user customizable setting would indeed be great, as some might want to use it to easily open Terminal, while others might want to launch another application.

Are you planning on adding this functionality in the near future?

lunaneff commented 2 years ago

I am checking both read and write permissions

I actually haven't thought of that before, but yeah, being certain that there's read permissions is a good idea. I also added a commit to the PR that changes the documentation to explicitly add read permissions. Wrapping the permission check in an else makes sense as well.

Are you planning on adding this functionality in the near future?

To be honest I kinda forgot about this extension, but this PR motivated me to work on it again. I might try to figure out how to do this today.

lunaneff commented 2 years ago

@jibsaramnim, I just added a hardware support table to the README. Could you tell me the exact model of your laptop so I can update it?