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 #294: Rapidly closing tabs with three-finger/middle click button re-opens closed tabs #295

Merged
merged 3 commits into from
Jul 7, 2019

Conversation

felix-andreas
Copy link
Member

@felix-andreas felix-andreas commented Jul 4, 2019

Fix for #294 and elementary/code#485: Dynamic notebook: Rapidly closing tabs with three-finger/middle
click button re-opens closed tabs

Changing child_revealed to reveal_child to make tab close-able by middle/three-finger click during
reveal animation. This prevents the creation of new tabs when trying to rapidly close multiple tabs.

Before

RAPID_CLICKING

After

RAPID_CLICKING_FIX

Fix Dynamic notebook: Rapidly closing tabs with three-finger/middle
click button re-opens closed tabs

Changing child_revealed to reveal_child to make tab closeable during
reveal animation.
@felix-andreas felix-andreas changed the title Fix #294: Fix for #294: Dynamic notebook: Rapidly closing tabs with three-finger/middle click button re-opens closed tabs Jul 4, 2019
@felix-andreas felix-andreas changed the title Fix for #294: Dynamic notebook: Rapidly closing tabs with three-finger/middle click button re-opens closed tabs Fix #294: Dynamic notebook: Rapidly closing tabs with three-finger/middle click button re-opens closed tabs Jul 4, 2019
@felix-andreas felix-andreas changed the title Fix #294: Dynamic notebook: Rapidly closing tabs with three-finger/middle click button re-opens closed tabs Fix #294: Rapidly closing tabs with three-finger/middle click button re-opens closed tabs Jul 4, 2019
@jlnr
Copy link
Member

jlnr commented Jul 6, 2019

@andreasfelix Definitely better than before! It still happens when I double-wheelclick on the border between two tabs, though. Do you think this is can/should be fixed within the same PR?

jlnr
jlnr previously approved these changes Jul 6, 2019
Copy link
Member

@jlnr jlnr left a comment

Choose a reason for hiding this comment

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

Solves the issue in most cases.

@felix-andreas
Copy link
Member Author

felix-andreas commented Jul 6, 2019

Unfortunately I think this is a different Issue. I have highlighted the size of the GranitsWidgetsTab and logged the enter and leave events:

Border_event

As one can see, the size of the GranitsWidgetsTab is slightly smaller than its visual size. This can also be seen at the event log: The signal get emitted when entering/leaving the blue highlighted area. When clicking on a Tab outside of this are, the signal gets directly passed to DynamicNotebook which leads to the restoring of the last closed tab.

I think this is related to the padding and border in the style sheet:

https://github.com/elementary/stylesheet/blob/ec23fec4a4bcd6577d2d2039eb24ae9bc9364577/elementary/gtk-3.0/gtk-widgets.css#L1670-L1688

Maybe it could be solved by changing notebook tab to notebook tab widgets grid, so the padding is within the Gtk.EventBox.

@felix-andreas
Copy link
Member Author

I have a other question: What is the rationale behind the close_button_is_visible method in the first place? Why shouldn't a tab be close-able by middle/three-finger when the close_button is not visible? Is this in any way needed for pinned tabs? I ask because of the line close_button_revealer.visible = !_pinned.

Otherwise it would be simpler to just remove the private bool close_button_is_visible () from the if statement:

if (e.button == 2 && close_button_is_visible ()) { // <-- remove close_button_is_visible ()
    e.state &= MODIFIER_MASK;

    if  (e.state == 0) {
        this.closed ();
    } else if (e.state == Gdk.ModifierType.SHIFT_MASK) {
        this.close_others ();
    }
} 

@jlnr
Copy link
Member

jlnr commented Jul 6, 2019

What is the rationale behind the close_button_is_visible method in the first place?

I have no clue. git blame leads to this commit: 13f290a

The referenced launchpad issue is about hiding the (x) buttons until you hover the tab, i.e. in the first commit where buttons weren't permanently visible.

I would guess that it was added out of unspecific defensiveness. Have you tried removing it? Does anything break? Less bugs and less code sounds good to me :)

@felix-andreas
Copy link
Member Author

I have completely removed the close_button_is_visible method as it always returns true in the if statement and isn't used anywhere else.

Copy link
Member

@jlnr jlnr left a comment

Choose a reason for hiding this comment

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

LGTM

@jlnr jlnr merged commit b357127 into elementary:master Jul 7, 2019
jmc-88 pushed a commit to jmc-88/granite that referenced this pull request Jul 8, 2019
…ck button re-opens closed tabs (elementary#295)

* Fix Dynamic notebook: Rapidly closing tabs with three-finger/middle
click button re-opens closed tabs
* Remove close_button_is_visible method
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.

2 participants