Remember directory for "remember last used directory" setting across sessions: fix issue 11326 and others #13887

molsonkiko · 2023-07-12T03:25:51Z

Would fix #11326
Would fix #10901
Would fix #4961
Would fix #4119

Previously config.xml did not remember the last used directory if "Remember last used directory" was chosen for the Default Directory setting. This meant that the last directory used for the Open/Save As/Rename forms was only tracked during the session, and was forgotten when the user quit Notepad++.

To test:

Open a session with files in at least three directories open (call them directories foo, bar, and baz)
Set the Default Directory setting to Remember last used directory
Use Open to open a file in directory foo.
Quit Notepad++.
Switch to a tab in directory bar.
Use the Open, Save As, or Rename forms. The starting directory should be foo, because that was the last used directory in the previous session.
While still staying in the bar file, set the Default Directory setting to baz using the text box.
Verify that the Open/Save As/Rename forms open in directory baz even when the current directory is bar or foo.
Switch to a tab in directory bar.
Set the Default Directory setting back to Remember last used directory.
Verify that the Open/Save As/Rename forms open in directory baz because that was the last used directory.
Set the Default Directory setting to Follow current document.
Switch to a tab in directory foo.
Verify that the Open/Save As/Rename forms all open in directory foo.
Set the Default Directory setting to Remember last used directory.
Switch to a tab in directory bar.
Verify that the Open/Save As/Rename forms open in directory foo because that was the last used directory.
Quit Notepad++.
Verify that the Open/Save As/Rename forms open in directory foo because that was the last used directory in the previous session.

Comments

Arguably it is somewhat undesirable that changing from Remember last used directory to Follow current document and then back to Remember last used directory causes forgetting of the directory that was chosen before the first change, but arguably this makes perfect sense because Remember last used directory is doing exactly what it claims to do, and remembering whatever directory was last chosen (whether implicitly or explicitly) by the user.

previously config.xml did not remember the last used directory if "Remember last used directory" was chosen for the Default Directory setting.

Yaron10 · 2023-07-12T19:41:26Z

@molsonkiko,

👍
Great.
It seems to be working like a charm.

Allow me one question.
Currently (master) if you set a custom-fixed folder (i.e. the third option in the "Default Directory" page), and then select one of the other two options - the fixed folder is remembered across sessions.
With this PR, if you set a fixed folder and then select "Last Used Directory" - the fixed folder is NOT remembered.
Wouldn't it be much better to remember both? Isn't it worth the extra code and a separate field in config.xml?

(In the "Search Engine" page - the custom engine is also remembered if you select one of the pre-defined options).

Thank you for your time and work.

molsonkiko · 2023-07-12T21:38:46Z

I can totally understand where you're coming from, but I don't think the added complexity of both source code and config.xml would be worth it. We'll see what Don Ho says, but my feeling is that minimizing the change to the source gives this PR the best chance of success.

…

On Wed, Jul 12, 2023, 12:41 PM Yaron10 ***@***.***> wrote: @molsonkiko <https://github.com/molsonkiko>, 👍 Great. It seems to be working like a charm. Allow me one question. Currently (master) if you set a custom-fixed folder (i.e. the third option in the "Default Directory" page), and then select one of the other two options - the fixed folder is remembered across sessions. With this PR, if you set a fixed folder and then select "Last Used Directory" - the fixed folder is NOT remembered. Wouldn't it be much better to remember both? Isn't it worth the extra code and a separate field in config.xml? (In the "Search Engine" page - the custom engine is also remembered if you select one of the pre-defined options). Thank you for your time and work. — Reply to this email directly, view it on GitHub <#13887 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ALAQAI2M5ML3OV4RPXMLNQDXP34XFANCNFSM6AAAAAA2G3PPCM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

Yaron10 · 2023-07-12T22:21:39Z

@molsonkiko,

My favorite PRs are those which reduce and optimize the code.
In this case, IMO, some extra code is justified for the optimal solution.

But I do respect your opinion.
And thank you again for your work. 👍

molsonkiko · 2023-07-13T03:03:51Z

@Yaron10
Now that I've experimented with the current behavior, I've come to the conclusion that the new behavior does in fact seem to be a true regression, so I'm definitely more inclined to agree with you that something should be done.

I still want to wait for Don Ho's opinion before I change the PR any more though.

Yaron10 · 2023-07-13T12:36:54Z

@molsonkiko,

👍
Thank you!

donho · 2023-07-15T22:52:15Z

Without talking about the implementation, do you guys think "Remember last used directory" should be through the sessions?
@Yaron10 @alankilborn @ArkadiuszMichalski @pryrt and anyone?

alankilborn · 2023-07-15T23:03:33Z

do you guys think "Remember last used directory" should be through the sessions?

This makes sense to me.

donho · 2023-07-15T23:21:35Z

It seems "straightforward" to the users that "Remember last used directory" is through the sessions. However, if "Default Open/Save file Directory" switch off from "Remember last used directory" to "Follow Current document" or to "Customized", and after some uses (open or save) it's switched back to "Remember last used directory" - what should be the value of "Remember last used directory"?

molsonkiko · 2023-07-15T23:51:13Z

I'm happy to try to implement whatever people decide is the most sensible way to implement this setting.

One thing that might help me with this task would be a better understanding of what nppGUI._defaultDir and nppGUI._defaultDirExp are for. I never came to understand the difference between their roles while I was making what I currently have.

Yaron10 · 2023-07-16T00:30:07Z

@donho,

what should be the value of "Remember last used directory"?

Why not use the current (master) behavior? - That is: when the user is selecting "Follow Current document" or "Custom", that folder is assigned to "Last used directory".
(NOTE: even without actually opening that folder).

@molsonkiko,

One thing that might help me with this task...

I hope @donho would be kind enough to help you with that.

mpheath · 2023-07-17T01:41:37Z

@molsonkiko

One thing that might help me with this task would be a better understanding of what nppGUI._defaultDir and nppGUI._defaultDirExp are for. I never came to understand the difference between their roles while I was making what I currently have.

You can see them in the Files changed tab that shows the patch.

::ExpandEnvironmentStrings(_nppGUI._defaultDir, _nppGUI._defaultDirExp, MAX_PATH);

_nppGUI._defaultDir is the IN value to be expanded. _nppGUI._defaultDirExp is the OUT pointer to a buffer that is Expanded. MAX_PATH is the buffer size.

_nppGUI._defaultDir would be what the user sets. example %UserProfile%\Desktop. _nppGUI._defaultDirExp would be the expanded value to be used as the default directory as like C:\Users\molsonkiko\Desktop .

Look for yourself at ExpandEnvironmentStringsW function which I used as a guide.

donho · 2023-07-17T12:25:27Z

By hacking _currentDirectory, it makes 2 regression:

While "Remember last used directory" is set, clicking the ... button in Find dialog doesn't follow anymore the current directory if "Follow current doc." is checked.
the 3rd choice (customized) of "Default Open/save file directory" is saved with the value of last used directory - it shouldn't be the case.

I will take care of the fix myself.

Yaron10 · 2023-07-17T17:02:25Z

@donho,

This issue has been opened for years.
You may not have been aware of that - I recently pinged @molsonkiko, and he kindly started working on it.
I feel that ignoring that would be highly disrespectful and ungrateful.

I hope you understand.

donho · 2023-07-17T17:36:04Z

@Yaron10

You may not have been aware of that - #11326 (comment) @molsonkiko, and he kindly started working on it.

I'm not aware of it indeed.
OK, I will do a small specification soon for @molsonkiko to modify this PR.

Yaron10 · 2023-07-17T17:47:53Z

@donho,

Thank you!
I truly respect and appreciate your decision. 👍

donho

Please consider the suggestion.

PowerEditor/src/Parameters.cpp

Now the last used directory remembered independently from the current directory and the default directory. Changing the default directory or changing the current directory has no impact on last used directory anymore. Both are also now in config.xml

molsonkiko · 2023-07-19T00:41:29Z

My most recent commit makes "last used directory" independent of the current file's directory and the default directory, as requested.

donho

Following the suggestions
Test your code to prevent PR from the regression.

PowerEditor/src/Parameters.h

molsonkiko · 2023-07-19T19:49:10Z

Good to know! Thanks for pointing that out, @rdipardo.

…

On Wed, Jul 19, 2023, 12:00 PM rdipardo ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In PowerEditor/src/Parameters.h <#13887 (comment)> : > @@ -923,6 +924,8 @@ struct NppGUI final DarkModeConf _darkmode; LargeFileRestriction _largeFileRestriction; + + TCHAR _lastUsedDir[MAX_PATH]; By putting _lastUsedDir in the middle of the struct, I'm forcing every plugin that interacts with that struct to make a bugfix patch [...] Adding a new member in *any* position is already a breaking change. If a plugin allocates insufficient memory for the bigger struct, the result will be an out-of-bounds write, leading to memory corruption if not a crash — Reply to this email directly, view it on GitHub <#13887 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ALAQAI4SGOCROPF3E64SMOTXRAVFZANCNFSM6AAAAAA2G3PPCM> . You are receiving this because you were mentioned.Message ID: ***@***.*** com>

donho

Please provide the reason for removing params.setWorkingDir(_events->getLastUsedFolder().c_str());.

If there's no valid reason to remove it, restore it.

PowerEditor/src/Parameters.h

PowerEditor/src/WinControls/OpenSaveFileDialog/CustomFileDialog.cpp

Yaron10 · 2023-07-20T10:28:39Z

@donho,

Thank you for your patient review and guidance. 👍

@molsonkiko,

Thank you for your work. 👍

Windows saves applications' last used folder.

I've just tested the new commit(s) here, and encountered what @alankilborn described in #11326 (comment):

However, on fresh restart of N++, I wanted to make a new PythonScript and when I went to Plugins > PythonScript > New Script and it wanted to create my new file in yesterday's "last directory". :-(

STR:
Use the binary of current master.
Select "remember last used directory".
Open a file located in "foo".
Exit NPP ("foo" is NOT in config.xml).

Use the binary of the latest commit here.
(IMPORTANT: the binary name must be the name used in the previous step; e.g. Notepad++.x64.Release.exe).
File -> Open.

Result:
The folder in the Open dialog is "foo".

Should the basic approach in this PR be re-considered?
The extra _lastUsedDir parameter and field in config.xml might not be necessary if we let Windows handle it.

In the current master, when the user is selecting "Follow Current document" or "Custom", that folder is assigned to "Last used directory".

This is NOT the behavior with this PR.

I think that either result is legit.
I'm just pointing it out.

donho · 2023-07-20T11:32:53Z

@Yaron10

The extra _lastUsedDir parameter and field in config.xml might not be necessary if we let Windows handle it.

My opinion is, let's keep the current implementation. Because just we cannot guarantee the Windows' behaviour is always unchanged.

Yaron10 · 2023-07-20T12:50:21Z

@donho,

Good point.

Also:

Windows "last used folder" seems to be per binary name.
Some "Cleaner" programs clear Windows "last used folder" by default.

While "Remember last used directory" is set, clicking the ... button in Find dialog doesn't follow anymore the current directory if "Follow current doc." is checked.

I didn't quite understand that.
With the latest commit here - if you delete the Folder ComboBox content, clicking the ... button you still don't get the current doc folder.
And if the Folder ComboBox is not empty - clicking the ... button you get the that folder even with the first commit here.

Yaron10 · 2023-07-20T13:34:52Z

STR:
Select "Last Used Directory".
Open a file in "foo".
Exit NPP.
Restore your older config.xml (or delete it).
Select "Last Used Directory".
File -> Open ("foo" is the folder).
Exit NPP.

Result:
The lastUsedDirPath field in config.xml is empty.

molsonkiko · 2023-07-20T14:46:28Z

Result: The lastUsedDirPath field in config.xml is empty.

It does seem like if I shut down Notepad++ with no config.xml (so NPP has to create a new config.xml), it initially behaves weirdly with lastUsedDirPath. I have no clue how to fix this, since it would appear that NPP is somehow creating the config.xml in a different way if it didn't already exist.

Yaron10 · 2023-07-20T15:00:52Z

@molsonkiko,

Thank you for looking into it.

donho · 2023-07-21T08:54:15Z

STR: Select "Last Used Directory". Open a file in "foo". Exit NPP. Restore your older config.xml (or delete it). Select "Last Used Directory". File -> Open ("foo" is the folder). Exit NPP.

Result: The lastUsedDirPath field in config.xml is empty.

@Yaron10
I cannot reproduce it.

Notepad++ v8.5.4   (64-bit)
Build time : Jul 21 2023 - 00:47:54
Path : C:\xxxxxxxx\notepad-plus-plus\PowerEditor\visual.net\x64\Debug\Notepad++.exe
Command Line : 
Admin mode : OFF
Local Conf mode : OFF
Cloud Config : OFF
OS Name : Windows 11 Home (64-bit)
OS Version : 22H2
OS Build : 22621.1992
Current ANSI codepage : 1252
Plugins : 
    mimeTools (2.6)
    NppConverter (4.3)
    NppExport (0.3)

donho · 2023-07-21T09:08:30Z

@Yaron10
I merge this PR now.
If you can reproduce the bug you mentioned, please create an issue and ping me.
I'll work on it myself. Thank you.

Yaron10 · 2023-07-21T13:03:13Z

@donho & @molsonkiko,

Thank you for your work. 👍

@donho,

I cannot reproduce it.

Tested with a fresh portable NPP.
#13914.

@molsonkiko,

I suppose you've managed to reproduce it, correct?

molsonkiko · 2023-07-21T15:36:39Z

@Yaron10 @donho

Thank you both so much for your help and feedback! Your work helping to create a good open-source community has been really important in inspiring me to keep contributing.

Yaron10 · 2023-07-21T18:12:36Z

@molsonkiko,

Thank you for your kind words. I appreciate it. 👍
I'd be really glad if you keep contributing.

@donho is the one who truly deserves many thanks & flowers for his on-going work on Notepad++, and for creating "a good open-source community".

save "remember last used dir" dir to xml

847c8e5

previously config.xml did not remember the last used directory if "Remember last used directory" was chosen for the Default Directory setting.

chcg added the enhancement Proposed enhancements of existing features label Jul 12, 2023

donho self-assigned this Jul 17, 2023

donho added the reject label Jul 17, 2023

donho closed this Jul 17, 2023

donho reopened this Jul 17, 2023

donho requested changes Jul 18, 2023

View reviewed changes

PowerEditor/src/Parameters.cpp Show resolved Hide resolved

donho removed the reject label Jul 18, 2023

donho requested changes Jul 19, 2023

View reviewed changes

PowerEditor/src/Parameters.h Outdated Show resolved Hide resolved

PowerEditor/src/Parameters.h Outdated Show resolved Hide resolved

revert getworkingdir, fix find in files bug

db6d760

donho requested changes Jul 20, 2023

View reviewed changes

PowerEditor/src/Parameters.h Outdated Show resolved Hide resolved

PowerEditor/src/WinControls/OpenSaveFileDialog/CustomFileDialog.cpp Outdated Show resolved Hide resolved

PowerEditor/src/WinControls/OpenSaveFileDialog/CustomFileDialog.cpp Show resolved Hide resolved

make changes requested by Don Ho

05e9ee0

woops, one last formatting change

d5833f5

donho closed this in ff2179a Jul 21, 2023

Yaron10 mentioned this pull request Jul 21, 2023

"Last Used Directory" may not be saved to config.xml #13914

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remember directory for "remember last used directory" setting across sessions: fix issue 11326 and others #13887

Remember directory for "remember last used directory" setting across sessions: fix issue 11326 and others #13887

molsonkiko commented Jul 12, 2023 •

edited

Yaron10 commented Jul 12, 2023

molsonkiko commented Jul 12, 2023 via email

Yaron10 commented Jul 12, 2023

molsonkiko commented Jul 13, 2023

Yaron10 commented Jul 13, 2023

donho commented Jul 15, 2023 •

edited

alankilborn commented Jul 15, 2023

donho commented Jul 15, 2023

molsonkiko commented Jul 15, 2023

Yaron10 commented Jul 16, 2023

mpheath commented Jul 17, 2023

donho commented Jul 17, 2023 •

edited

Yaron10 commented Jul 17, 2023

donho commented Jul 17, 2023

Yaron10 commented Jul 17, 2023

donho left a comment

molsonkiko commented Jul 19, 2023

donho left a comment

molsonkiko commented Jul 19, 2023 via email

donho left a comment

Yaron10 commented Jul 20, 2023

donho commented Jul 20, 2023

Yaron10 commented Jul 20, 2023 •

edited

Yaron10 commented Jul 20, 2023

molsonkiko commented Jul 20, 2023

Yaron10 commented Jul 20, 2023

donho commented Jul 21, 2023

donho commented Jul 21, 2023

Yaron10 commented Jul 21, 2023

molsonkiko commented Jul 21, 2023

Yaron10 commented Jul 21, 2023

Remember directory for "remember last used directory" setting across sessions: fix issue 11326 and others #13887

Remember directory for "remember last used directory" setting across sessions: fix issue 11326 and others #13887

Conversation

molsonkiko commented Jul 12, 2023 • edited

To test:

Comments

Yaron10 commented Jul 12, 2023

molsonkiko commented Jul 12, 2023 via email

Yaron10 commented Jul 12, 2023

molsonkiko commented Jul 13, 2023

Yaron10 commented Jul 13, 2023

donho commented Jul 15, 2023 • edited

alankilborn commented Jul 15, 2023

donho commented Jul 15, 2023

molsonkiko commented Jul 15, 2023

Yaron10 commented Jul 16, 2023

mpheath commented Jul 17, 2023

donho commented Jul 17, 2023 • edited

Yaron10 commented Jul 17, 2023

donho commented Jul 17, 2023

Yaron10 commented Jul 17, 2023

donho left a comment

Choose a reason for hiding this comment

molsonkiko commented Jul 19, 2023

donho left a comment

Choose a reason for hiding this comment

molsonkiko commented Jul 19, 2023 via email

donho left a comment

Choose a reason for hiding this comment

Yaron10 commented Jul 20, 2023

donho commented Jul 20, 2023

Yaron10 commented Jul 20, 2023 • edited

Yaron10 commented Jul 20, 2023

molsonkiko commented Jul 20, 2023

Yaron10 commented Jul 20, 2023

donho commented Jul 21, 2023

donho commented Jul 21, 2023

Yaron10 commented Jul 21, 2023

molsonkiko commented Jul 21, 2023

Yaron10 commented Jul 21, 2023

molsonkiko commented Jul 12, 2023 •

edited

donho commented Jul 15, 2023 •

edited

donho commented Jul 17, 2023 •

edited

Yaron10 commented Jul 20, 2023 •

edited