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

Fix error 'metaWindow.get_title is not a function' #344

Merged
merged 3 commits into from
Feb 14, 2024

Conversation

p1gp1g
Copy link
Contributor

@p1gp1g p1gp1g commented Jan 24, 2024

This was the cause of ghosts windows with dynamic workspaces: Closing a window in a workspace reduced the number of workspaces above it. Windows in these workspaces considered that there was still another window, and finished tiling with a ghost window.

Error log:

JS ERROR: TypeError: metaWindow.get_title is not a function
removeFloatOverride@file:///var/home/user/.local/share/gnome-shell/extensions/[email protected]/lib/extension/window.js:106:30
windowDestroy@file:///var/home/user/.local/share/gnome-shell/extensions/[email protected]/lib/extension/window.js:1623:12
_destroyWindowDone@resource:///org/gnome/shell/ui/windowManager.js:1555:21
onStopped@resource:///org/gnome/shell/ui/windowManager.js:1523:39
_makeEaseCallback/<@resource:///org/gnome/shell/ui/environment.js:84:22
_easeActor/<@resource:///org/gnome/shell/ui/environment.js:173:64
@resource:///org/gnome/shell/ui/init.js:21:20

to make it easier to read
This was the cause of ghosts windows with dynamic workspaces:
Closing a window in a workspace reduced the number of workspaces
above it. Windows in these workspaces considered that there was
still another window, and finished tiling with a ghost window.
@p1gp1g
Copy link
Contributor Author

p1gp1g commented Feb 7, 2024

Gentle ping @bennypowers @juarezr for the review. Except the variable renaming, the only change is https://github.com/forge-ext/forge/pull/344/files#diff-ecc5aba1b19feb12aedd8037846a95297082072be1a704814195edb468f846dfR1614-R1617

Once merge we can publish a new release :)

lib/extension/window.js Outdated Show resolved Hide resolved
this.tree.removeNode(nodeWindow);
this.renderTree("window-destroy-quick", true);
this.removeFloatOverride(nodeWindow, false, true);
this.removeFloatOverride(nodeWindow.nodeValue, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Has the signature of this function changed it just the variable name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The signature of removeFloatOverride has been changed in another patch and hasn't been changed here

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hey @bennypowers, do you have any concerns on the function signature change?

Hey @p1gp1g, I can update your role so you can merge. Just need someone to test looks OK or need evidence (screencap) that this is working fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

No the signature change doesn't bother me; it's just not immediately clear from this PR why this change was made here

If OP wants bonus marks, split that line into a separate commit with a detailed message

But that's not something which should hold up the merge.

If other reviewers have run this and found it acceptable, I'm good with it

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alright, just found out that I can add per repository role maintain access to all of you in settings.

Co-authored-by: Benny Powers - עם ישראל חי! <[email protected]>
Copy link
Contributor

@bennypowers bennypowers left a comment

Choose a reason for hiding this comment

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

I haven't found the time to run this and give it proper qa, but discussions in this PR thread have assured me that the changes here are good to go

@jmmaranan jmmaranan merged commit 9290955 into forge-ext:main Feb 14, 2024
@jmmaranan
Copy link
Collaborator

No worries thanks @bennypowers. @p1gp1g - you might want to give it one more look from main. Then I can release over the weekend.

Thank you for all your contributions

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.

3 participants