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

gTile: coherently update ui on monitor change, move gnome terminal #434

Closed
wants to merge 11 commits into from

Conversation

settenop
Copy link

@settenop settenop commented Dec 14, 2022

Hi, the extension had some unintuitive behaviour for me in multimonitor so i made the following changes:

  • if after a keypress the active grid changed: redraw the selected area UI
  • allowed to start the selection with left/up keys

Then i noticed the terminal was not moving, so:

  • I've added a call to metaWindow.move_frame after the move_resize_frame operation
  • since the terminal can't be resized freely, but move_resize_frame should make its size less or equal than the desired size, i've distribuited evenly the remaining space

This should fix #425

Could you review it pls? @Gr3q @shuairan

Only tested in Cinnamon 5.4.12

Copy link
Contributor

@Gr3q Gr3q left a comment

Choose a reason for hiding this comment

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

Please build the 3.8 version as well.

Also, while you are at it, could you fix a bug that are introduced?

Showing the Grid UI right after restarting Cinnamon breaks.
The problem is that the current monitor is not updated before it tries to get the current grid, here: https://github.com/settenop/cinnamon-spices-extensions/blob/master/gTile%40shuairan/src/base/app.ts#L285

The fix is to move this line https://github.com/settenop/cinnamon-spices-extensions/blob/master/gTile%40shuairan/src/base/app.ts#L288 above the other one I linked.

@Gr3q
Copy link
Contributor

Gr3q commented Dec 15, 2022

I'm sorry, can you revert my proposed fix? It breaks other behaviour, like mouse switching between windows.

Apologies, I will fix that issue properly later

@Gr3q
Copy link
Contributor

Gr3q commented Dec 15, 2022

Apart from that, everything is ok

@settenop
Copy link
Author

pls note i had to run npm audit fix and npm update otherwise the builds were failing with ERR_OSSL_EVP_UNSUPPORTED

@settenop
Copy link
Author

settenop commented Dec 15, 2022

I'm sorry, can you revert my proposed fix? It breaks other behaviour, like mouse switching between windows.

Apologies, I will fix that issue properly later

Could we fix it using always the same grid when showGridOnAllMonitors is false?
In app.ts we could change CurrentGrid() to this:

  public get CurrentGrid(): Grid {
    if (!this.config.showGridOnAllMonitors) {
      return this.grids[0];
    }
    const grid = this.grids.find(x => x.monitor.index == this.currentMonitor.index)!;
    return grid;
  }

Edit:
We should also update the lines you referred to as follows

    if (!this.config.showGridOnAllMonitors) {
      if (this.CurrentGrid) {
        this.CurrentGrid.ChangeCurrentMonitor(this.monitors[this.focusMetaWindow.get_monitor()]);
      }
    }

@Gr3q
Copy link
Contributor

Gr3q commented Dec 17, 2022

Had a bit of time to look today, the problem is that the function is executed twice, here first:

https://github.com/settenop/cinnamon-spices-extensions/blob/master/gTile%40shuairan/src/base/app.ts#L120-L121

So these 2 lines can be removed. Of course a null check would definitely help, even tho CurrentGrid should never be null

@settenop
Copy link
Author

settenop commented Dec 17, 2022

yes, I've found some issues too.

  • the hover in the grid elements are triggered even if the mouse hover the grid because the grid is moving while the mouse don't (for example when !this.config.showGridOnAllMonitors).
  • when the mouse is used to move the grid to another monitor by clicking on a free desktop area nemo is selected as the target application.
  • while the grid is opened changing focus to another window of the same application will not change the target window. (fixed in f6ca8f3)

Copy link
Contributor

@Gr3q Gr3q left a comment

Choose a reason for hiding this comment

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

I forgot about this PR. Btw the proper way to fix the build is to update the webpack version to the latest.

agentx3 pushed a commit to agentx3/cinnamon-spices-extensions that referenced this pull request Apr 12, 2023
@koenhendriks
Copy link

@Gr3q Any update on merging this? Would really like to use gTile with my terminal :D

@Gr3q
Copy link
Contributor

Gr3q commented Apr 12, 2023

@brownsr is the one who reviews and merges PRs

@koenhendriks
Copy link

My bad @Gr3q , Figured you were the one to ask as you are set as Reviewer on the PR

@brownsr You think you can take a look and merge the branch soon :D ?

@brownsr
Copy link
Member

brownsr commented Jul 2, 2023

Sorry for the long wait, I hadn't seen there was a change waiting here. Unfortunately there are conflicts. Can you resolve these, and then I should be able to merge.

Regards

Simon

@claudiux
Copy link
Member

@settenop This PR is now too old. Please create a new one after synchronizing your master branch fork.

@claudiux claudiux closed this Jul 21, 2023
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.

gTile@shuairan: Gtail does not move gnome-terminal window @ LM21
5 participants