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

Add shortcuts for height and opacity #72

Merged
merged 1 commit into from
Aug 11, 2021
Merged

Conversation

jacksongoode
Copy link
Contributor

@jacksongoode jacksongoode commented Aug 8, 2021

Default keybindings for increasing/decreasing size are take from Guake, opacity bindings are unset.

This would close #70.

Copy link
Member

@amezin amezin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, the implementation is correct. Please fix setting and action names, and try to get rid of code duplication.

After that:

  1. Please don't change package-lock.json
  2. Squash all commits into one

application.js Outdated Show resolved Hide resolved
appwindow.js Outdated Show resolved Hide resolved
glade/prefs.ui Outdated Show resolved Hide resolved
appwindow.js Outdated Show resolved Hide resolved
@jacksongoode
Copy link
Contributor Author

jacksongoode commented Aug 9, 2021

Almost done, but it seems there are errors that's appearing now:

Can't update stage views actor MetaSurfaceActorX11 is on because it needs an allocation.
Can't update stage views actor MetaWindowActorX11 is on because it needs an allocation.
Can't update stage views actor MetaWindowGroup is on because it needs an allocation.

This appears everytime the hotkey is used to increase/decrease the size (silent on transparency changes).

@amezin
Copy link
Member

amezin commented Aug 9, 2021

Almost done, but it seems there are errors that's appearing now:

Can't update stage views actor MetaSurfaceActorX11 is on because it needs an allocation.
Can't update stage views actor MetaWindowActorX11 is on because it needs an allocation.
Can't update stage views actor MetaWindowGroup is on because it needs an allocation.

This appears everytime the hotkey is used to increase/decrease the size (silent on transparency changes).

You may ignore these warnings for now. Most likely your code isn't responsible for them.

Do they appear if you open preferences and drag the "Window size" slider?

@jacksongoode
Copy link
Contributor Author

jacksongoode commented Aug 9, 2021

Almost done, but it seems there are errors that's appearing now:

Can't update stage views actor MetaSurfaceActorX11 is on because it needs an allocation.
Can't update stage views actor MetaWindowActorX11 is on because it needs an allocation.
Can't update stage views actor MetaWindowGroup is on because it needs an allocation.

This appears everytime the hotkey is used to increase/decrease the size (silent on transparency changes).

You may ignore these warnings for now. Most likely your code isn't responsible for them.

Do they appear if you open preferences and drag the "Window size" slider?

Yes, I will ignore them.

One final issue I've noticed is that changes to background-opacity if it is initially set to 1.0 will not appear until the window is moved or selected (focused). Like:

  1. Set transparency to 1.0
  2. Hide/close window
  3. Open window
  4. Change transparency in settings
  5. Window does not change until focused by selection or move

I believe this is a result of this line. I don't think there is any reason for this? @amezin I've removed this condition to fix the behavior. Let me know if this is incorrect.

Copy link
Contributor Author

@jacksongoode jacksongoode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me so far.

appwindow.js Outdated Show resolved Hide resolved
appwindow.js Outdated Show resolved Hide resolved
@jacksongoode
Copy link
Contributor Author

Not sure about the test for gnome-wayland-nested-dual-monitor - is there something I can do @amezin?

Copy link
Member

@amezin amezin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If window-maximize is true, resizing should start from 1.0.

  1. Maximize the window (F11 by default)
  2. Try to decrease its height.

It will resize to the original (unmaximized) size minus HEIGHT_MOD.

IMO it should resize to 1.0 - HEIGHT_MOD instead.

glade/prefs.ui Outdated Show resolved Hide resolved
@amezin
Copy link
Member

amezin commented Aug 10, 2021

Not sure about the test for gnome-wayland-nested-dual-monitor - is there something I can do @amezin?

No, the test failure is unrelated to your changes. Tests rely on various timeouts too much, and some configurations thus fail from time to time.

appwindow.js Outdated Show resolved Hide resolved
appwindow.js Outdated Show resolved Hide resolved
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.

Shortcut for increasing/decreasing terminal height
2 participants