Manuel-Kehl / Go-For-It

A stylish to-do list with built-in productivity timer. NOTE: all contributions should go to Jonathan Moerman's fork because this is where active development is taking place: https://github.com/JMoerman/Go-For-It
http://manuel-kehl.de/projects/go-for-it/
GNU General Public License v3.0
516 stars 51 forks source link

Better handling of exit #28

Closed voldyman closed 9 years ago

voldyman commented 9 years ago

this is a better way of handling window close event

Manuel-Kehl commented 9 years ago

I will have a closer look at it, when I am at home ;-) Thanks for the pull request!

Manuel-Kehl commented 9 years ago

First of all, thank you for your ongoing engagement in the project ;-) I am sorry, but I do not quite agree on using iconify (i.e. minimize) for the following reasons:

Do you agree with my points?

voldyman commented 9 years ago

noise behaves this way.

the other option would be to hide the window instead of minimizing it.

The current behavior is of deleting the existing window and recreating it in hidden state, the two options we have are to either minimize it when the timer is running or to hide the window.

i would prefer hiding, minimizing tells the user that the application has not quit maybe we don't need to inform the user

Manuel-Kehl commented 9 years ago

Mhh strange. Noise closes (or hides) instead of minimizing on my system (up-to-date Freya), even if a song is playing.

Nevertheless, I would prefer hiding the window instead of minimizing it, for the reasons mentioned above. I do agree that hiding makes more sense then destroying and recreating it. I simply did not think of that solution due to my lack of Gtk and Vala experience :laughing:

By the way: Micah Ilbery already designed a potential tray icon, so we could utilize that for informing the user that a background task is running. Following this approach we could incorporate the remaining time + task description in combination with timer controls in the context menu.

voldyman commented 9 years ago

fixed the iconify thing, sorry got busy with uni.

Manuel-Kehl commented 9 years ago

@voldyman It's all right. I am in the very same situation right now :wink: I tested your changes. It appears to break the code responsible for remembering the window geometry, as save_win_geometry is connected to MainWindow.delete_event. I tried connecting it to MainWindow.hide but it did not have the desired effect.

As I am also pretty busy with university related work, I do not have the time to further investigate. You can, of course, feel free to look into it, if you find the time.

Manuel-Kehl commented 9 years ago

All right. This is almost perfect :wink: The only regression left to be fixed is the fact, that it still does not save the position, if the timer is running. This is strange, as you call save_win_geometry before even considering the timer's state. Maybe it is strangely related to the fact that the function returns true in that case and thus stops other handlers from reacting to delete_event, although that does not make sense to me :confused: :question:

voldyman commented 9 years ago

hmm, it is working for me. did you close all running instances of 'Go For It' before testing? ;)

Manuel-Kehl commented 9 years ago

I even rebuilt everything and restarted the computer. But still the position is not being saved when the timer is running. I'd suppose to leave the pull request like that for now and further investigate into the phenomenon, when we have more time. Are you fine with that?

voldyman commented 9 years ago

sure

Manuel-Kehl commented 9 years ago

Ok I did one more experiment. The position does get saved, but not restored properly, when unhiding the window! Maybe there is a signal to connect to / function to override, where we can call restore_win_geometry?

Manuel-Kehl commented 9 years ago

By connecting restore_win_geometry to the Window's show signal, the aforementioned problem is fixed, but it has a strange effect on the initial placement of new windows: The window is displayed in the left corner at first and then moved to the corresponding spot, instead of directly appearing at the correct position.

Manuel-Kehl commented 9 years ago

I think I fixed it! I am going to merge your pull request and commit my additions. Thanks for this contribution @voldyman and sorry for me nagging so much :laughing:

I simply want to avoid all sorts of potential regressions.