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

Now shows warning before closing unsaved note #25

Closed
wants to merge 5 commits into from
Closed

Now shows warning before closing unsaved note #25

wants to merge 5 commits into from

Conversation

R4H33M
Copy link
Contributor

@R4H33M R4H33M commented Dec 14, 2019

Needed to refactor the quit logic of the app, but I think now the code is cleaner.
Fixes #13

@R4H33M
Copy link
Contributor Author

R4H33M commented Dec 15, 2019

I think I understood what you suggested, and I have changed my code to follow the coding guidelines.

@R4H33M
Copy link
Contributor Author

R4H33M commented Dec 15, 2019

Man these invisible trailing tabs get me everytime. Thanks for pointing them out.

@owenca
Copy link

owenca commented Dec 15, 2019

A couple of issues:

  • Label the Save button "Save", not "Save first", to be consistent with other applications.
  • After Save, the program should quit. (See Pe and Koder, for example.)

@R4H33M
Copy link
Contributor Author

R4H33M commented Dec 15, 2019

I've changed it exactly how you said, Ready for review!

@R4H33M
Copy link
Contributor Author

R4H33M commented Dec 15, 2019

We good now, chief(s)? :))

@humdingerb
Copy link
Member

I checked out the PR again and now, after clicking "Save" in the alert, the file dialog blinks into existence before the app cquits without saving...

@R4H33M
Copy link
Contributor Author

R4H33M commented Dec 15, 2019

That's really strange. It didn't do that before, and now I double checked it again on another machine and it works without any flaws. Did you test the latest version of the PR?
From the code, one would expected it to work as intended. I don't see any issues, do you?


(obligatory meme ;)))
image

@humdingerb
Copy link
Member

I just removed the checked out branch and re-fetched it. Still seeing the same behaviour on gcc2. Using gcc8, it crashes with https://linx.li/takenotes-99812-debug-15-12-2019-19-23-02.report

@R4H33M
Copy link
Contributor Author

R4H33M commented Dec 15, 2019

Weird. I honestly have no clue why this would happen.

@owenca
Copy link

owenca commented Dec 15, 2019

It crashes on x86 (gcc8) for me too. Also, on gcc2, if you open an existing note and then quit without editing it at all, you can't quit without seeing the dialog first. @R4H33M can you fix it?

Copy link

@owenca owenca left a comment

Choose a reason for hiding this comment

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

It doesn't work when opening an existing note and then quit without changing the note.

@owenca
Copy link

owenca commented Dec 15, 2019

One more bug: the dialog doesn't pop up if you change the note and then close the window. This is probably what @humdingerb was talking about.

@humdingerb
Copy link
Member

How do we want to continue on here?
I played around a bit more and found that there's no crash when building with DEBUGGER=TRUE. Probably I had that on when I tested before...

The crash also disappears on gcc2 when built normally when changing the if (!fSaveMessage) in QuitRequested() to if (fSaveMessage == NULL). On gcc8 it still crashes though...

@owenca
Copy link

owenca commented Dec 17, 2019

I think the two non-crash bugs should be fixed first and then the crash tracked down.

The crash also disappears on gcc2 when built normally when changing the if (!fSaveMessage) in QuitRequested() to if (fSaveMessage == NULL).

That's weird.

@R4H33M
Copy link
Contributor Author

R4H33M commented Dec 17, 2019

Oh hello. I was just working on fixing these issues, and through my testing I see very strange and unique behaviour - these bugs do not seem straightforward to fix.

They seem really hard, but I will continue attempting to fix this bug, and will update you as I go along. Any support would be greatly appreciated. Also, it might be a few days before we reach a potential solution.

@scottmc
Copy link
Member

scottmc commented Dec 24, 2019

There's been a few other merged pull requests on TakeNotes since this pull request was first opened. Please check it with the latest upstream changes to see if it affects this code at all.

@scottmc
Copy link
Member

scottmc commented Jan 6, 2020

So what's the status on this one? There's been a couple of other merged pull requests recently so I'm assuming it needs to at least be rebased?

@R4H33M
Copy link
Contributor Author

R4H33M commented Jan 6, 2020

Agreed.
I will still try to find a solution for these bugs, and it's quite an important addition so I don't think we should drop it quite yet.

@R4H33M
Copy link
Contributor Author

R4H33M commented Jan 14, 2020

I am closing this PR, as I will be starting over with fixing issue #13.
A new PR will be opened soon.

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

Successfully merging this pull request may close these issues.

Show save dialog on unsaved notes
4 participants