@xomx @pnedev
Could you review this PR please?
As @pnedev has said, there's nothing more we can do about it. So I remove the part of deletion session backup file - user can get back their before last session which is under the name "session.xml.inCaseOfCorruption.bak".

@donho ,
Looks good to me. 👍

@donho
I have debugged this PR and found these problems (I put all my notes here at one place, if you want me to do also a review in the code, please let me know):

1. Instead of the used L"Saving session error - restore backup", I would rather choose the wording L"Saving session error - restoring from the backup:" in the NppParameters::writeSession msgbox.

2. Wrong ReplaceFile use (or I do not get your intent).

   • I simulated a session.xml write failure - while running under the debugger, immediately after the session.xml file was successfully saved in the NppParameters::WriteSession, I paused the N++ and externally corrupted the contents of the session.xml file (e.g. to simulate the NUL-bytes problem...) and then let the code continue to the ReplaceFile part
   • the above resulted in: no session.xml file at all, session.xml.inCaseOfCorruption.bak file containing the incorrect session.xml data and session.xml.fail2Load file created containing the right (previous backup) data - is this what you intended?

3. Consider please also the following addition of the content-corruption check of the written session.xml in the NppParameters::WriteSession (this immediately reveals e.g. the possible NULs...)

  //
  // Double checking: prevent written session file corrupted while writting
  //
  if (sessionSaveOK)
  {
  	TiXmlDocument* pXmlSessionCheck = new TiXmlDocument(sessionPathName);
  	sessionSaveOK = pXmlSessionCheck->LoadFile();
  	if (sessionSaveOK)
  	{
  		Session sessionCheck;
  		sessionSaveOK = getSessionFromXmlTree(pXmlSessionCheck, sessionCheck);
  	}
  	delete pXmlSessionCheck;
  }
  else

4. Adjust please also the session.xml loading part in the NppParameters::load() (and maybe also in the NppParameters::loadSession)

   • If, for any reason, it is not possible to write a new session.xml file (or it is then damaged somehow) but there will be a valid backup copy session.xml.inCaseOfCorruption.bak, after restarting the N++, the backup will not be used automatically, but an empty session will be created, which will even overwrite the existing backup file when the N++ gets closed!
   • By the above I mean this scenario - the N++ user usually discovers this problem by opening the N++ and finding that their session is empty. If he/she closes the N++ at this point before starts looking for the backup, that backup file could be destroyed by overwriting with the current empty-session when the N++ quits...
   • so in case of a session.xml loading fail, but when the session.xml.inCaseOfCorruption.bak exists, show also a modal msgbox informing the user that the session.xml.inCaseOfCorruption.bak has been found and will be used instead of the session.xml file (+ add such loading code from the session.xml.inCaseOfCorruption.bak)

5. Complement the existing session.xml loading code part above (4.) with the same content checking as used for the NppParameters::loadSession func.

   • Is there a reason why the NppParameters::loadSession is not re-used at the startup session.xml loading place ?

6. Consider adding of the !nppParam.isEndSessionCritical() before any possible msgbox in the NppParameters::WriteSession

   • this critical-flag is also set when the shutdown is already inevitable
   • because of the N++ session.xml is written at the app-exit, we should take this into account and not use the blocking modal msgbox when Windows shutdown is already in progress

One more general note - improper use of the the PathFileExists WINAPI in the NppParameters::WriteSession (but this is a common problem in the N++ code base, more in the #14839 ).

Edit: When you will be doing some changes in the code - there is one typo: isAllLaoded

@xomx
Thank you. Very good code review.

Instead of the used L"Saving session error - restore backup", I would rather choose the wording L"Saving session error - restoring from the backup:" in the NppParameters::writeSession msgbox.

Done in the latest commit.

Wrong ReplaceFile use (or I do not get your intent).

Fixed in the latest commit - tested also with your debug break point way then modify session.xml.

Consider please also the following addition of the content-corruption check of the written session.xml in the NppParameters::WriteSession (this immediately reveals e.g. the possible NULs...)

Done in the latest commit - it detects more the session file error indeed.

Adjust please also the session.xml loading part in the NppParameters::load() (and maybe also in the NppParameters::loadSession)

Good thinking!
Done in the latest commit. Tested also with your debug break point way then modify session.xml.

Complement the existing session.xml loading code part above (4.) with the same content checking as used for the NppParameters::loadSession func.

NppParameters::loadSession()method is for loading session manually via File command "Load Session..." & it contains messagebox warning, whereas inNppParameters::load()` there should be no message warning, and there are more specific stuff as added in the current PR. However, I will check in the future for the possibility of refactoring.

Consider adding of the !nppParam.isEndSessionCritical() before any possible msgbox in the NppParameters::WriteSession

Done in the latest commit.

@donho
I have found some problems while debugging the new code and I will do an adjustment PR for that.

The N++ startup NppParameters::load() session-loading code does not handle right the situation where there is a session.xml.inCaseOfCorruption.bak file but no session.xml file. My patch will be likewise the:

Also it should check for a possible ReplaceFile error return.