Quit Fix

[nathanial]

Uncommenting the GTK delete_event code appears to fix adobe/brackets#4611

I tested on Arch Linux 64-bit, using the latest brackets-shell and brackets versions.

[redmunds]

@nathanial You'll need to agree to the Brackets CLA.

[redmunds]

@nathanial Here are the key shutdown scenarios:

1. Brackets is open with no documents open (File > Close All). Click [x] button once to quit.

2. Brackets is open with 1 or more documents open. All changes are saved to disk (File > Save All). Click [x] button once to quit.

3. Brackets is open with 1 or more documents open with unsaved changes. Click [x] button, Brackets prompts to "Save", "Don't Save", "Cancel".
   a. Click "Save" should save changes to disk and quit
   b. Click "Don't Save" should discard changes and quit
   c. Click "Cancel" should keep changes in memory and abort quit process

   Once you are in state following 3c, you should be able to follow all of the other scenarios (1, 2, 3a, 3b, & 3c).

4. You can use Debug > New Window to open additional Brackets windows. This is in the Debug menu because it's an experimental feature, but it should be tested to make sure nothing really bad happens. Try closing from both original and new windows.

[nathanial]

@redmunds Aha, cool. I will test these scenarios. Also, I agreed to the CLA.

[nathanial]

@redmunds Ok, I ran through all the cases and they all appear to work. For scenario 4, the only oddness is that closing the original window closes all the windows.

[redmunds]

@nathanial

closing the original window closes all the windows

Actually, that is the expected behavior.

Just need to find someone with Linux Dev environment setup to review this PR.
cc @JeffryBooher @ingorichter @jasonsanjose

[JeffryBooher]

probably not a good idea to call browser->GetHost() after calling browser->GetHost()->CloseBrowser(). This may crash in future versions since theoretically it should delete the "browser" pointer on close.

[nathanial]

@JeffryBooher I adjusted the code and re-tested it. Appears to work.

[peterflynn]

@ingorichter Said he could re-review this soon, so assigning him

[ingorichter]

@nathanial I've tested this on Ubuntu 14.04 TLS 32Bit and it creates a core dump when I exit Brackets. I'm starting Brackets from the command line and that's the reason I was seeing the message about the core dump. Starting from whatever file manager doesn't give any indication about a core dump. Otherwise it seems that Brackets will exit after one click on the close button. Which is an improvement already.

[ingorichter]

@nathanial what have you done to compile brackets-shell on Arch Linux? I'm getting a Release/libcef.so: undefined reference to 'gcry_control@GCRYPT_1.2' when I run grunt build.

[nathanial]

@ingorichter I've just been doing grunt build. It's possible that either brackets-shell has changed (I notice some new commits have been made since I did this pull request) or perhaps I have a different version of gcrypt. I'll check when I get home.

[nathanial]

@ingorichter I'll also investigate the core dump.

[nathanial]

@ingorichter According to ldconfig I have these versions of gcrypt on my machine.

    libgcrypt.so.20 (libc6,x86-64) => /usr/lib/libgcrypt.so.20
    libgcrypt.so.20 (libc6) => /usr/lib32/libgcrypt.so.20
    libgcrypt.so.11 (libc6,x86-64) => /usr/lib/libgcrypt.so.11
    libgcrypt.so (libc6,x86-64) => /usr/lib/libgcrypt.so
    libgcrypt.so (libc6) => /usr/lib32/libgcrypt.so

I don't seem to get any errors when I run grunt build with this version of the code. I might have done some troubleshooting when I very first got it to compile, but I don't remember. I'll try compiling it on a fresh VM to see if there are any extra steps that need to be taken.

[ingorichter]

@nathanial I was able to remove a install a version of libgcrypt and compile Brackets. But there is still something wrong, since it's not starting properly. :-(

[radorodopski]

I don't think you need the code on line 532. Isn't this the hack, which was supposed to be removed once CEF was fixed?

I un-commented the code and removed that line (and the else, too, since it seems to not be needed anymore) and Brackets now behaves as expected (i.e. given that no file was modified, Brackets exited after just one click of the close button).
I also edited a file and clicked close, then Brackets asked me to save (or discard the changes) - clicking "Cancel" kept Brackets from closing, clicking "Save" saved the changes, and clicking "Don't Save" discarded the changes.
Brackets exited clean during all of the above scenarios (or didn't exit, after clicking "Cancel").

Is there something I'm missing, so we actually need line 532 for some reason?

P.S.: I forgot to mention - I'm using Ubuntu 14.04.1 x64 (LTS) with latest updates.

[radorodopski]

@ingorichter , do you think you can test this without line 532?
You mentioned you got a core dump with the fix, maybe it has something to do with sending a delete_event to a window which is already in the process of closing (or already closed) because of the code on line 532.

Edit:
File --> Quit resulted in the following errors in the terminal:
[1024/064618:ERROR:browser_main_loop.cc(212)] GLib-GObject: invalid cast from 'GtkExpandedContainer' to 'GtkWindow'
[1024/064618:ERROR:browser_main_loop.cc(212)] Gtk: IA__gtk_window_present_with_time: assertion 'GTK_IS_WINDOW (window)' failed

I assumed this was because of this fix, but then I ran the latest stable release of Brackets from command line just for sanity check and it turns out the same messages are dumped even when using just "browser->GetHost()->CloseBrowser(true);" in the CloseWindow() function (i.e. without this fix).
This needs more investigation...

[radorodopski]

Could someone point me to the right information?
I need to know how File --> Quit (from the HTML menu under Linux) is being handled.

Edit:
Contrary to what I thought, File --> Quit works fine despite the interesting error messages, which are probably an issue with CEF, not Brackets. So, just for closure, I'd like to know what happens when a user clicks File --> Quit...

[ingorichter]

@radorodopski let me have a look later this week. We are currently working on 1.0 release and time is very limited.

[radorodopski]

Hi @ingorichter,

What worked for me was just uncommenting the following lines in appshell_extensions_gtk.cpp:

GtkWidget* hwnd = gtk_widget_get_toplevel (browser->GetHost()->GetWindowHandle() );
if(gtk_widget_is_toplevel (hwnd))
    gtk_signal_emit_by_name(GTK_OBJECT(hwnd), "delete_event");
else
    browser->GetHost()->CloseBrowser(true);

Let me know if this also works for you.

[peterflynn]

@ingorichter Do you still have cycles to review this more, or should we unassign?

[ingorichter]

Let me check this, since I ran into the issues with my VM right away last time. This shouldn't take that much time.

[jasonsanjose]

@nathanial @ingorichter I tried merging this with our current work with CEF 2171. Seems to work great for me on my Ubuntu 14.04 64-bit VM, see #503.

[prksingh]

@abose Can you have a look at this?

[aaulia]

Any news about merging this PR? Using 1.7 on Arch (from AUR) right now, and the issue still exists.

[ficristo]

@aaulia are you able to test it and verify if it does work?
(this should be tested on top of the linux-1547 branch and not master)

Otherwise I suggest to you to use brackets-electron.
A bit more context is here