Did this work correctly in previous versions of TDE or KDE 3.5.10?

(Odpověď na komentář #1)
> Did this work correctly in previous versions of TDE or KDE 3.5.10?

Unfortunately I do not know - I walked from KDE 3.5.10 to Trinity 3.5.12.

I don't use KNotes and I am unfamiliar. Would you please post the steps I need to perform to test/replicate this bug? Thanks!

(Odpověď na komentář #3)
> I don't use KNotes and I am unfamiliar. Would you please post the steps I need
> to perform to test/replicate this bug? Thanks!

1. Run KNotes.
2. Create a new note, write something to note and let it open.
3. Log out (with saving session)
4. Log in and wonder why note did not remain open :)

Confirmed.

Works fine in KDE 3.5.10. :-(

Does this work correctly in Trinity 3.5.12?

(Odpověď na komentář #6)
> Does this work correctly in Trinity 3.5.12?

No, it does not work properly in 3.5.12. Therefore I set in the bug report version 3.5.12. As it was before 3.5.12 I have not got to try.

As restoration works in 3.5.10 and not in 3.5.12 or GIT, then we know the timeline when the bug was introduced. I'll take a guess the bug was introduced in this patch:

http://git.trinitydesktop.org/cgit/tdepim/commit/knotes?id=cc29364f06178f8f6b457384f2ec37a042bd9d43

That patch was merged between 3.5.11 and 3.5.12. Perhaps a C++ guru can look at the patch and notice the cause?

(In reply to comment #7)
> (Odpověď na komentář #6)
> > Does this work correctly in Trinity 3.5.12?
> 
> No, it does not work properly in 3.5.12. Therefore I set in the bug report
> version 3.5.12. As it was before 3.5.12 I have not got to try.

It works for me here on Ubuntu, but I always save the session on logout.

I wonder if this is really a bug, as not saving the session would imply discarding all open windows...

I set the session manager to Restore manually saved session. I opened and edited a KNotes note. I left Konqueror open (minimized) as a second method to verify the session is restored.

From the TDE menu I selected Save Session. I then logged out and logged in. Although KNotes restarted and the KNotes icon was in the system tray, no note appeared on the desktop. Konqueror appeared minimized. Selecting Knotes from the system tray caused the note to appear.

(Odpověď na komentář #9)
> It works for me here on Ubuntu, but I always save the session on logout.
> 
> I wonder if this is really a bug, as not saving the session would imply
> discarding all open windows...

I use autosave session. For other programs than KNotes I not noticed the problem. (Debian Squeeze)

Created attachment 617 [details]
A simple one-line fix

Tested and works well. Saving the configuration is done in other places. So in closing it was unnecessary => undesirable.

(In reply to comment #12)
> Created attachment 617 [details]
> A simple one-line fix
> 
> Tested and works well. Saving the configuration is done in other places. So in
> closing it was unnecessary => undesirable.

Just to verify, if I edit a note, then immediately close KNotes, will the note be saved or lost?

(Odpověď na komentář #13)
> (In reply to comment #12)
> > Created attachment 617 [details] [details]
> > A simple one-line fix
> > 
> > Tested and works well. Saving the configuration is done in other places. So in
> > closing it was unnecessary => undesirable.
> 
> Just to verify, if I edit a note, then immediately close KNotes, will the note
> be saved or lost?

Saved, because all notes are saved during the termination KNotes. As I said - saving them anyway carried out elsewhere.

So the bug was introduced in the patch I referenced in Comment 8.

Slavek's attached patch restores the code to the same as in 3.5.10, which does not exhibit the bug for me on my systems.

That raises the question of why the one-line change was believed necessary from whomever created that original massive patch (Suse? Ubuntu? Debian?). And why the bug is not seen in Ubuntu.

Push the patch to GIT?

(Odpověď na komentář #15)
> So the bug was introduced in the patch I referenced in Comment 8.
> 
> Slavek's attached patch restores the code to the same as in 3.5.10, which does
> not exhibit the bug for me on my systems.
> 
> That raises the question of why the one-line change was believed necessary from
> whomever created that original massive patch (Suse? Ubuntu? Debian?). And why
> the bug is not seen in Ubuntu.
> 
> Push the patch to GIT?

Yes, it is true. It's in the patches you've said. I looked into the patch before, as you said it well, I looked into it again. When I am there the one-line caught the eye. :)

I do not know who had a rationale, there's added saving. But apparently it caused a problem. In response to closeEvent not distinguish why is the window closes. Such a reason is also the end of session. So I first verified that the saving is done elsewhere and then the I just re-throw this line.

For me has a vote for commit it in the GIT.

(In reply to comment #15)
> So the bug was introduced in the patch I referenced in Comment 8.
> 
> Slavek's attached patch restores the code to the same as in 3.5.10, which does
> not exhibit the bug for me on my systems.
> 
> That raises the question of why the one-line change was believed necessary from
> whomever created that original massive patch (Suse? Ubuntu? Debian?). And why
> the bug is not seen in Ubuntu.
> 
> Push the patch to GIT?

Go ahead and push.  No metadata exists explaining why that one-line change was made, so it may not have been needed at all.

Tim

Pushed to GIT in hash 533f494f.

Resolved.

Thank you!

After resolving the error 922 (d2f8fca9 patch) notes are now closed during the testing, whether it is possible to log off. However, if the log off is interrupted, notes will remain closed. Hence notes should not be closed so soon.

(In reply to comment #19)
> After resolving the error 922 (d2f8fca9 patch) notes are now closed during the
> testing, whether it is possible to log off. However, if the log off is
> interrupted, notes will remain closed. Hence notes should not be closed so
> soon.

This goes much deeper, into the core design of the TDE session manager (smserver) itself.

When you try to log off/shut down/etc., TDE sends out a signal for all open applications to save and close (phase 1).  kate, kwrite, and a host of other applications can intercept this call and prevent logout, but they cannot prevent the phase1 shutdown signal from reaching other applications such as knotes.

Essentially what I am saying is that there is no mechanism in place to "undo" the phase1 shutdown request.  I don't even know what that would look like to be honest, as it would involve relaunching part of the still-unsaved session.

A temporary fix might be to make knotes respond to the phase1 signal by saving its notes but not exiting, and then respond to the phase2 signal by shutting down without saving.

(Odpověď na komentář #20)
> This goes much deeper, into the core design of the TDE session manager
> (smserver) itself.
> 
> When you try to log off/shut down/etc., TDE sends out a signal for all open
> applications to save and close (phase 1).  kate, kwrite, and a host of other
> applications can intercept this call and prevent logout, but they cannot
> prevent the phase1 shutdown signal from reaching other applications such as
> knotes.
> 
> Essentially what I am saying is that there is no mechanism in place to "undo"
> the phase1 shutdown request.  I don't even know what that would look like to be
> honest, as it would involve relaunching part of the still-unsaved session.
> 
> A temporary fix might be to make knotes respond to the phase1 signal by saving
> its notes but not exiting, and then respond to the phase2 signal by shutting
> down without saving.

The problem is that knotes on signal phase1 closes notes, but shutting down on signal phase2. Closing notes should therefore not be in phase1, but in phase2 - along with shutting down. It must do both simultaneously.

> The problem is that knotes on signal phase1 closes notes, but shutting down on
> signal phase2. Closing notes should therefore not be in phase1, but in phase2 -
> along with shutting down. It must do both simultaneously.

Gotcha.  This should be a relatively simple fix unless the knotes developers did something weird in their session handlers.

Tim

Created attachment 629 [details]
Do not close notes during saving session

Really - fix was quite simple.

It is interesting that our patch was useful also for upstream KDE 4.x - see:
https://bugs.kde.org/show_bug.cgi?id=300456

(In reply to comment #24)
> It is interesting that our patch was useful also for upstream KDE 4.x - see:
> https://bugs.kde.org/show_bug.cgi?id=300456

That's good to hear!

Did you commit this to GIT yet?  If so, please post the GIT short hash to the bug report and mark it RESOLVED FIXED.

Thanks!

Tim

Fixed in GIT hash c48253a.