ProgerXP / Notepad2e

Word highlighting, simultaneous editing, split views, math evaluation, un/grep, comment reformatting, UAC elevation, complete regexps (PCRE), Lua lexers, DPI awareness and more (XP+)
Other
375 stars 52 forks source link

History is cleared if "Save recent files on exit" isn't checked in Open Recent File dialog #401

Open Anton-V-K opened 2 years ago

Anton-V-K commented 2 years ago

Test scenarion:

  1. Open any file
  2. Alt+H or File => Recent (History)...
  3. Press Cancel in the dialog

Actual: the menu item Recent (History) is greyed, so the history isn't available any longer.

Expected: the files history should be preserved when the dialog is cancelled.

BTW after pressing OK in the dialog (switching to a file), the history is also cleared and only current file is kept there. Is this by design?

ProgerXP commented 2 years ago

BTW after pressing OK in the dialog (switching to a file), the history is also cleared and only current file is kept there. Is this by design?

I don't see this behaviour. Normally, History is preserved during the process lifetime (if you don't have INI file saved) or forever (if you do). Can you record a video?

Anton-V-K commented 2 years ago

Reproducible with Notepad2e-R213-7158575-x86 on Windows 10 21H1 64-bit: https://user-images.githubusercontent.com/20116984/153059976-85b59072-a12b-4da7-81a6-88d410809aff.mp4

ProgerXP commented 2 years ago

I can't reproduce this on Win10 or Win7. Does it happen with Notepad2?

Anton-V-K commented 2 years ago

Reproducible also with last stable release R134. This functionality works fine in Notepad2 4.2.25 (x86)

The issue is related to the code fragment in SaveSettings: https://github.com/ProgerXP/Notepad2e/blob/71585758099ec07d61dd14ba806068c0d937efd3/src/Notepad2.c#L6177

  if (n2e_CanSaveINISection(bCheckSaveSettingsMode, SSM_ALL)
      || (n2e_CanSaveINISection(bCheckSaveSettingsMode, SSM_RECENT) && bSaveRecentFiles))
  {
    // Cleanup unwanted MRU's
    if (!bSaveRecentFiles)
    {
      MRU_Empty(pFileMRU);
      MRU_Save(pFileMRU);
    }
    else
    {
      MRU_MergeSave(pFileMRU, TRUE, flagRelativeFileMRU, flagPortableMyDocs);
    }
  }

MRU_Empty just cleans the list if bSaveRecentFiles is false (it is the option "Save recent files on exit" in Open Recent File dialog, which is off by default).

PS: to me this looks like an unexpected behavior, probably partially incorrectly implemented, refer to #101

ProgerXP commented 2 years ago

PS: to me this looks like an unexpected behavior, probably partially incorrectly implemented, refer to #101

Looks like a bug to me as well but it wasn't introduced by #101. Notepad2 has MRU_Empty() calls in WM_DESTROY (see the initial commit in this repo). However, these calls were moved into SaveSettings() by 0a09720 which changed their logic from "run once on exit" to "run every time settings save" making the fact MRUs were cleared visible to the user. I don't remember what issue the commit was part of (perhaps one of code review tasks like #53).

Let's leave this for @cshnik. To reproduce, uncheck Save recent files on exit in History and set Save settings on exit to All.

cshnik commented 2 years ago

Fixed. The issue was resolved by the partial rollback of the changes from 709f966e (initial commit for #101). The points are:

  1. "Save recent files on exit" checkbox maps to "Settings> Remember Recent Files" menu option.
  2. SaveSettings()-call is useless when processing "Open Recent File" dialog destroy since this option will be saved on application quit or directly via "Settings> Save Settings> Save Settings Now" menu command.
  3. Recent Files list should not be implicitly cleaned out during the application work.