search

arelange / gnome-shell-extension-hibernate-status

Gnome Shell extension that adds a hibernate/hybrid suspend button in Status menu.
GNU General Public License v2.0
156 stars 58 forks source link

Add hybrid sleep option toggle and make confirmation dialog optional #81

Closed jibsaramnim closed 11 months ago

jibsaramnim commented 2 years ago

(Sorry this one took a bit longer, life got in the way)

I'm submitting this as a draft for right now as I think it might need a bit more cleaning up and checking, but would like to open this up for you (and possibly others) to help do this, and get some initial feedback.

screenshot of the new, rather barren looking settings panel

I had to make some fundamental modifications to the code as this introduces an actual settings view -- and because I had to add this anyway, I figured I can just include #2 straight away too. With the implementation of making the hibernation confirmation dialog optional, I also went ahead and modified the actual hibernation button label to include the typical "..." to indicate there is a next action/step to the option, just like how "Restart..." is shown. I thought that minor detail would be nice to add.

I'd like to tackle the remaining open issue #23 as that'd actually be convenient for me too (I use suspend-then-hibernate on one of my devices that currently can't support full S3 sleep), but I feel like this might actually benefit from not being a separate option in the list, but instead replace the existing system suspend option. What do you think? Would a separate option be better, or should we look into the replace option?

Happy to take whatever feedback you can share, and I've enabled the allow edits checkbox, so please feel free to work along with me on this if there's anything you'd like to touch up.

Many thanks in advance!

jibsaramnim commented 1 year ago

@p91paul Gentle nudge, just in case this one flew by under the radar. Please let me know if there's anything wrong or missing here, I'm happy to work with you to get it merged and updated! :)

p91paul commented 1 year ago

terribly sorry, my work life has been quite difficult recently and gave me little free time. I'll try and test/merge this

jibsaramnim commented 1 year ago

No problem Paolo, I totally understand. Thank you so much for finding some time whenever you can! :)

p91paul commented 1 year ago

Hi, I reviewed the pull request and I would merge it, but I preferred to give priority to #84 because the extension is now broken for gnome 43 users, and there are some conflicts now... are you able to resolve them? I can't right now because I'm still on Gnome 42 and wouldn't be able to test until Arch Linux updates Gnome.

Just a note, when contributing to another project I'd ask you to disable automatic formatting on your editor. The pull request was somewhat hard to review because it was full of unrelated changes to formatting (quotes, wrapping, etc.)

p91paul commented 1 year ago

@saulotoledo I really have no plans more than trying and keeping this alive. I use Gnome on my free time only, work forces me to be on Windows, and my free time is quite limited. This is why when a new version comes out and code changes are needed, I just drop compatibility for older version. To get new features, backwards compatibility, and a quicker support for new shell versions, a new maintainer is needed.

jibsaramnim commented 1 year ago

Thanks so much for looking through this, and for adding some feedback too! I’ll reply in more detail and work on an update when I’m not on mobile, but I wanted to join in about this bit:

But what do you plan to do now: keep this only from Gnome 43, or add some backward compatibility support as well?

I only took a quick look at the other PR, and it seems like it should be fairly straightforward in most cases to if/else the new and old ways, to maintain backwards compatibility too. Would that be an idea? Especially initially it would be nice if people using distros that don’t update as quickly can keep using this extension, too.

saulotoledo commented 1 year ago

@jibsaramnim, I started a rebase at https://github.com/saulotoledo/gnome-shell-extension-hibernate-status/tree/feat/rebased-jibsaramnim-add-hybrid-sleep-toggle to check the changes, but I wasn't able to test it yet. You can use it as a reference if helps. I also added missing translation files for pt and pt_BR (you can copy them in your PR when you rebase it, or I can send a PR later if you prefer). Could you also take the chance and add the translation for "Hybrid Sleep" to the Dutch language?

Regarding the backward compatibility, given the comment from @p91paul above (thanks for your answer, @p91paul), I would keep this for Gnome 43 now and discuss it in a separate task. I believe this settings box is a nice thing to have asap, and breaking it into smaller tasks would be better. @jibsaramnim, do you have an environment with gnome 43 to test your changes? I can help with that soon (during the week will be challenging, but I can find some time).

jibsaramnim commented 1 year ago

Apologies for the radio silence, I've had a particularly busy few weeks. I haven't had a chance to dive back into this yet, but will try to make time very soon!

@jibsaramnim, I started a rebase at https://github.com/saulotoledo/gnome-shell-extension-hibernate-status/tree/feat/rebased-jibsaramnim-add-hybrid-sleep-toggle to check the changes, but I wasn't able to test it yet. You can use it as a reference if helps. I also added missing translation files for pt and pt_BR (you can copy them in your PR when you rebase it, or I can send a PR later if you prefer). Could you also take the chance and add the translation for "Hybrid Sleep" to the Dutch language?

I haven't looked at this rebase yet -- To make sure I understand it correctly, is the idea for me to rebase off of this one, then continue making requested changes/fixes/etc.?

slaclau commented 11 months ago

These features have been implemented by another PR(#106)