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

With VTE 0.54, hitting X button of one tab closes all tabs #61

Closed
yan12125 opened this Issue Sep 15, 2018 · 5 comments

Comments

Projects
None yet
3 participants
@yan12125
Member

yan12125 commented Sep 15, 2018

Original bug report: https://bugs.archlinux.org/task/60035

gdb indicates that running gtk_notebook_remove_page(GTK_NOTEBOOK(terminal->notebook), term->index) for the closing tab triggers another child-exited signal, thus causing the next tab to be closed. This procedure is going through recursively and then all tabs are closed.

The relevant change in VTE might be this: https://gitlab.gnome.org/GNOME/vte/commit/0276859709692ec53f35a8cc158d7553a195c730

@egmontkob

This comment has been minimized.

Show comment
Hide comment
@egmontkob

egmontkob Sep 16, 2018

Inside terminal_close_button_event(), replacing terminal_child_exited_event(vte, 0, term); by gtk_widget_destroy(term->vte); seems to work for me at first glimpse. I'm not sure if this is the proper solution.

(Perhaps should be conditional to vte-0.54, leaving the old code path for older vte versions, I haven't tested that.)

Note though that terminal_close_button_event()'s signature is confusingly broken. The first argument it receives and forwards to terminal_child_exited_event() is a GtkButton (the close button) and not a VteTerminal. Looks like a more thorough check of the code around these bits is desirable.

egmontkob commented Sep 16, 2018

Inside terminal_close_button_event(), replacing terminal_child_exited_event(vte, 0, term); by gtk_widget_destroy(term->vte); seems to work for me at first glimpse. I'm not sure if this is the proper solution.

(Perhaps should be conditional to vte-0.54, leaving the old code path for older vte versions, I haven't tested that.)

Note though that terminal_close_button_event()'s signature is confusingly broken. The first argument it receives and forwards to terminal_child_exited_event() is a GtkButton (the close button) and not a VteTerminal. Looks like a more thorough check of the code around these bits is desirable.

@egmontkob

This comment has been minimized.

Show comment
Hide comment
@egmontkob

egmontkob Sep 16, 2018

Probably a fix along the lines of https://bugs.launchpad.net/sakura/+bug/1790317 would be better, that is, to explicitly disconnect the child-exited signal before removing vte from the widget hierarchy.

(I'd let you guys play with it, I'm not planning to further work on this issue.)

egmontkob commented Sep 16, 2018

Probably a fix along the lines of https://bugs.launchpad.net/sakura/+bug/1790317 would be better, that is, to explicitly disconnect the child-exited signal before removing vte from the widget hierarchy.

(I'd let you guys play with it, I'm not planning to further work on this issue.)

@HelixBot HelixBot closed this in b7c07dd Sep 16, 2018

@medicalwei

This comment has been minimized.

Show comment
Hide comment
@medicalwei

medicalwei Sep 16, 2018

Contributor

Note though that terminal_close_button_event()'s signature is confusingly broken. The first argument it receives and forwards to terminal_child_exited_event() is a GtkButton (the close button) and not a VteTerminal. Looks like a more thorough check of the code around these bits is desirable.

This is addressed in a4eaef4 as well.

However, for this issue please test and reopen if disconnecting the signal on child-exited doesn't help.

Contributor

medicalwei commented Sep 16, 2018

Note though that terminal_close_button_event()'s signature is confusingly broken. The first argument it receives and forwards to terminal_child_exited_event() is a GtkButton (the close button) and not a VteTerminal. Looks like a more thorough check of the code around these bits is desirable.

This is addressed in a4eaef4 as well.

However, for this issue please test and reopen if disconnecting the signal on child-exited doesn't help.

@medicalwei medicalwei reopened this Sep 20, 2018

@yan12125

This comment has been minimized.

Show comment
Hide comment
@yan12125

yan12125 Sep 21, 2018

Member

Anything wrong with a4eaef4? That works fine for me.

Member

yan12125 commented Sep 21, 2018

Anything wrong with a4eaef4? That works fine for me.

@medicalwei

This comment has been minimized.

Show comment
Hide comment
@medicalwei

medicalwei Sep 21, 2018

Contributor

My mistake. I was running on wrong binary to test it and found out the bug "persists".

Contributor

medicalwei commented Sep 21, 2018

My mistake. I was running on wrong binary to test it and found out the bug "persists".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment