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

Regression of v8.5.0, still found in v8.5.3 tracking write protection #13742

Closed
cs-hv opened this issue Jun 5, 2023 · 42 comments
Closed

Regression of v8.5.0, still found in v8.5.3 tracking write protection #13742

cs-hv opened this issue Jun 5, 2023 · 42 comments
Assignees
Labels

Comments

@cs-hv
Copy link

cs-hv commented Jun 5, 2023

Description of the Issue

When a write protected file is opened and the write protection is removed, notepad++ still shows that the file is write protected and editing is not possible. Write protection has to be manually removed using the corresponding menu before the file can be edited and re-written.
Background: We use SVN for managing our source codes an when a file shall be edited, it has to be locked using a context menu from a plugin of notepad++. When the file is locked, the write protection is cleared. Notepad++ fails to track this change correctly.
This happens only for the first file being editing this way.
With v8.4.9 the behaviour is as exptected.

Steps to Reproduce the Issue

  1. Open a file that is write protected.
  2. Remove write protection (i.e. using explorer).

Expected Behavior

Write protection removed, file can be edited.

Actual Behavior

Notepad++ still shows the file being write protected, editing not possible.

Debug Information

Notepad++ v8.5.3 (32-bit)
Build time : May 15 2023 - 06:07:16
Path : C:\Program Files (x86)\Notepad++\notepad++.exe
Command Line :
Admin mode : OFF
Local Conf mode : OFF
Cloud Config : OFF
OS Name : Windows 10 Enterprise (64-bit)
OS Version : 22H2
OS Build : 19045.2965
Current ANSI codepage : 1252
Plugins :
GhNppExec ()
mimeTools (2.9)
NppConverter (4.5)
NppExport (0.4)

@alankilborn
Copy link
Contributor

Presume if a file has "write protection", it means that a file's "Read-only" attribute is "ON" when examined in Explorer, e.g.:

image


When I try an experiment with this on 8.5.3, I obtain @cs-hv's Expected Behavior, i.e., when I switch the input focus back to N++ after doing @cs-hv 's step 2, the tab bar's icon for the file immediately changes from the "lock" icon to the "checkmark" icon ("alternate icons" in effect), and the file can be edited.

@cs-hv
Copy link
Author

cs-hv commented Jun 5, 2023

Yes, exactly. As I'm in Germany, the terminus is "Schreibschutz" (this applies to notepad++ as well as to the explorer) what I translated to "write protection" but has the same meaning/function as "Read-only".

The bug only applies to the access of "the very first file every day", a circumstance which makes the bug difficult to reproduce. Even exiting and restarting notepad++ does not make the bug re-occur. Notepad++ seams to miss the attribute change of the file only once a day.

However when using notepad++ v8.4.9 (or earlier) the bug does not occur.

NB: As I wrote in the initial post, we are using the GhNppExec plugin to access the "SVN lock" command, which in turn removes the read-only attribute from the corresponding file after locking the file. The read-only attribute is removed but notepad++ is not in sync with the file system.

@donho donho added the not reproducible Issues that can't be reproduced, with the given information, using the latest Notepad++ version label Jun 5, 2023
@donho
Copy link
Member

donho commented Jun 5, 2023

@cs-hv
I cannot reproduce in v8.5.3.
Open a normal file (not ready-only) - the disk icon is blue.
Then go to explorer, change the attribute of opened file to read only. After switching back to Notepad++, the disk color change to gray. Go to explorer again, and remove the read only attribute. Switch back to Notepad++, the disk color become blue again.

My Debug Info:

Notepad++ v8.5.3   (64-bit)
Build time : May 15 2023 - 06:09:36
Path : C:\Program Files\Notepad++\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.1778
Current ANSI codepage : 1252
Plugins : 
    mimeTools (2.9)
    NppConverter (4.5)
    NppExport (0.4)

@cs-hv
Copy link
Author

cs-hv commented Jun 6, 2023

Thank you for your efforts.
I did further testing. Using the GhNppExec plugin as described before is not necessary, the error can also be reproduced using the explorer. I did the following this morning:

  1. Opened a source file using notepad++'s open dialogue.
  2. Remove the Read-Only flag using the explorer.
  3. Notepad++ does NOT reflect the change (i.e. icon stays gray).
  4. Closing notepad++ and restarting notepad++ still shows the file would be read-only!
  5. Closing notepad++ and restarting it through double-click on the corresponding file still shows the file would be read-only!
  6. Closing the file within notepad++ and re-open the file: Now the icon is shown blue and file can be modified.

Something must have been changed from v8.4.9 to v8.5.0 because with v8.5.0 it was the first time we could observe this strange behaviour.

@donho
Copy link
Member

donho commented Jun 6, 2023

I cannot reproduce the bug.
Are there someones who can reproduce it?
If yes, please provide the detail description to reproduce this issue and provide your debug info.

@forcepc
Copy link

forcepc commented Jun 6, 2023

Hi,
Yes, it happens to me all the time with the newer versions, never had the issue before.
Steps to reproduce:

  1. Open file with read only attribute. Icon is greyed out in notepad++
  2. Change attribute to remove read only protection
  3. File is still seen as read only in notepad++. Using the command Reload from Disk doesn't help.

OS is Win 10 20H2 if that can help.

The reason I'm seeing the bug is that our version control software has all files read-only by default.
Thanks!

@donho
Copy link
Member

donho commented Jun 6, 2023

@forcepc
Thank you.
Could you provide your debug info please?

@Yaron10
Copy link

Yaron10 commented Jun 6, 2023

@cs-hv,

What's your setting in Preferences -> MISC?

תמונה

@cs-hv
Copy link
Author

cs-hv commented Jun 7, 2023

Hi Yaron10,
the setting is the same as shown in your screenshot (i.e. "Enable", both check boxes blank).

@forcepc
Copy link

forcepc commented Jun 7, 2023

Here you go:

Notepad++ v8.5.3   (64-bit)
Build time : May 15 2023 - 06:09:36
Path : C:\Program Files\Notepad++\notepad++.exe
Command Line : c:\my_file.sv
Admin mode : OFF
Local Conf mode : OFF
Cloud Config : OFF
OS Name : Windows 10 Enterprise (64-bit)
OS Version : 20H2
OS Build : 19042.2965
Current ANSI codepage : 1252
Plugins : 
 ComparePlugin (2.0.2)
 mimeTools (2.9)
 NppConverter (4.5)
 NppExport (0.4)

As mentioned earlier, no issues with 8.4.9.

@forcepc
Copy link

forcepc commented Jun 7, 2023

I checked the setting that @Yaron10 mentioned and mine was set to "Disable". I changed it to "Enable" and now notepad++ seems to correctly detect the read only change...
Now I'm wondering, was that setting set to "Enable" by default in 8.4.9 and "Disable" in 8.5.x? Was there a bug in < 8.5 where it was detecting the status change when it should not have?
How to explain that with the setting set to "Disable" a "Reload from Disk" does not detect the read/only correctly whereas closing and re-opening the file does?

@Yaron10
Copy link

Yaron10 commented Jun 7, 2023

Hi @cs-hv,

the setting is the same as shown in your screenshot

I have no idea why it won't work properly on your machine.

@forcepc,

I changed it to "Enable" and now notepad++ seems to correctly detect the read only change...

Great.

How to explain that with the setting set to "Disable" a "Reload from Disk" does not detect the read/only correctly whereas closing and re-opening the file does?

I see your point.
But one can argue that Reload is only reloading the content.

@cs-hv
Copy link
Author

cs-hv commented Jun 9, 2023

I did tests on a second PC and the probleme is reproducible on it.
I even did uninstall notepad++ completely from this PC and made a clean reinstall - but that didn't solve the problem either.
Here the debug info of the second machine:
Notepad++ v8.5.3 (32-bit)
Build time : May 15 2023 - 06:07:16
Path : C:\Program Files (x86)\Notepad++\notepad++.exe
Command Line :
Admin mode : OFF
Local Conf mode : OFF
Cloud Config : OFF
OS Name : Windows 10 Enterprise (64-bit)
OS Version : 22H2
OS Build : 19045.3031
Current ANSI codepage : 1252
Plugins :
GhNppExec ()
mimeTools (2.9)
NppConverter (4.5)
NppExport (0.4)

However I noticed the following:
When a file is opened by e.g. double clicking it in windows explorer, the tracking of the read-only flag seams to work.
When the file is opened from within notepad++ (i.e. using Ctrl-O dialogue) the tracking of the read-only flag fails for these files. It seams to depend on the method how the file is opened.

@Yaron10
Copy link

Yaron10 commented Jun 9, 2023

@cs-hv,

When a file is opened by e.g. double clicking it in windows explorer, the tracking of the read-only flag seams to work.
When the file is opened from within notepad++ (i.e. using Ctrl-O dialogue) the tracking of the read-only flag fails for these files.

We're making some progress. :)
I can't reproduce it that way either.

This happens only for the first file being editing this way.

I'd suggest that you test this point a bit more and report again.

@cs-hv
Copy link
Author

cs-hv commented Jun 9, 2023

Hi Yaron10,
the following statement is not true:
This happens only for the first file being editing this way.
Every file opened this way suffers from the problem.
I'll be on holiday for some time so please don't be surprised if I don't reply during this period.

@Yaron10
Copy link

Yaron10 commented Jun 9, 2023

Hi @cs-hv,

No problem. Enjoy you holiday.

@ozone10
Copy link
Contributor

ozone10 commented Jun 11, 2023

@Yaron10 @cs-hv
seems like Notepad++ uses two type of "read-only" flags. One from file (set e.g. from explorer) and another internal one (read-only only in Notepad++).

When opening via open dialog both flags are applied. So removing from explorer/clear read-only flag cmd will only remove flag from file, but not Notepad++'s flag.

Unfortunately I don't have time to do investigation which code does this until next month.

@donho
Copy link
Member

donho commented Jun 11, 2023

What @ozone10 is talking about is this option (set RO only in Notepad++) :

image

I don't think file attribute RO is interfered by Notepad++'s RO.
But just in case, @cs-hv @forcepc could you check if this option is checked for the concerning files?

@Yaron10
Copy link

Yaron10 commented Jun 11, 2023

@ozone10,

When opening via open dialog both flags are applied. So removing from explorer/clear read-only flag cmd will only remove flag from file, but not Notepad++'s flag.

Can you reproduce @cs-hv's issue?

@donho,

Those two commands might be slightly confusing.
How about changing them to Read-Only (in Notepad++) & Clear System Read-Only Flag?

@alankilborn
Copy link
Contributor

How about changing them to Read-Only (in Notepad++) & Clear System Read-Only Flag?

I like:

  • Set Read-Only in Notepad++
  • Set Read-Only File System Attribute
  • Clear Read-Only File System Attribute

@Yaron10
Copy link

Yaron10 commented Jun 13, 2023

@alankilborn,

Your wording is better. 👍

Set Read-Only File System Attribute

Do you mean a new option to set that attribute?

@alankilborn
Copy link
Contributor

Do you mean a new option to set that attribute?

Ha. Yeah. I snuck that in. :-)

@Yaron10
Copy link

Yaron10 commented Jun 13, 2023

@alankilborn,

Would you like to open a FR for that?
If it's accepted - it can also be one toggleable command, correct?

@ozone10
Copy link
Contributor

ozone10 commented Jun 13, 2023

@donho
open via Open dialog (ctrl+O)
image
drag & drop
image

@Yaron10
Copy link

Yaron10 commented Jun 13, 2023

I can reproduce it too.

@cs-hv,

My apologies.
You were right: the bug does not exist in v8.4.9 and was introduced in v8.5.0.

@donho
Copy link
Member

donho commented Jun 13, 2023

Do you mean a new option to set that attribute?

Removing RO file attribute is convenient for users, but setting file attribute in Notepad++ is pointless (IMO).
So this feature won't be included in Notepad++, sorry.

@donho
Copy link
Member

donho commented Jun 13, 2023

@ozone10 Thank you!
The culprit found: 5e2f5d7

@donho donho added accepted and removed not reproducible Issues that can't be reproduced, with the given information, using the latest Notepad++ version labels Jun 13, 2023
@donho donho self-assigned this Jun 13, 2023
@donho donho closed this as completed in a266bd5 Jun 13, 2023
@Yaron10
Copy link

Yaron10 commented Jun 13, 2023

@donho,

The culprit found: 5e2f5d7
...
I will use BufferID test = doOpen(fns.at(i).c_str()); for fixing it.

👍
Thank you.

@alankilborn
Copy link
Contributor

Removing RO file attribute is convenient for users, but setting file attribute in Notepad++ is pointless (IMO).

Points:

  • It is nice when software has symmetrical functions (in this case, clear/set)
  • In the past, users have been confused that they can clear but not set the attribute
  • A user accidentally clears the attribute with an errant mouse-click on the menu; now they are left wondering the best way to "undo" that (sadly, I myself make more errant mouse-clicks than I used to)
  • User is creating a file with some short content and wants to set it as read-only after that; user can't believe that they have to find an alternate means of setting the attribute than using their favorite text editor to do it

So this feature won't be included in Notepad++, sorry.

No problem to me; I have long-ago scripted the behavior, so it is available to me.

@alankilborn
Copy link
Contributor

The culprit found: 5e2f5d7

It is good to find the culprit, but unfortunately for me, it was a change I made (and no one else saw anything wrong with it at the time).
Can anyone explain exactly how it caused the current issue, because I don't see it. :-(

@Yaron10
Copy link

Yaron10 commented Jun 14, 2023

@alankilborn,

To your eyes only. :)

Sometimes amateurs get lucky.
At the time I thought (or rather "felt") it was better to use BufferID test = doOpen(fns.at(i).c_str());. I therefore thumbed up your issue but not your PR.
And that's why I couldn't reproduce it earlier: I tested it with a build containing the modified line (a mistake obviously).

It turns out that CustomFileDialog::isReadOnly() returns true in some cases where isReadOnly in doOpen() should be false.

@alankilborn
Copy link
Contributor

It turns out that CustomFileDialog::isReadOnly() returns true in some cases where isReadOnly in doOpen() should be false.

So, really then, my change logically was reasonable, but it exposed a different problem; I saw some question Don asked of mere-human about that (I think), but I can't find where that was now. OK, I can let it go.

@Yaron10
Copy link

Yaron10 commented Jun 14, 2023

So, really then, my change logically was reasonable, but it exposed a different problem;

That's correct.
That line (before your PR) was obviously wrong, and mere-human was not around to explain it.

I saw some question Don asked of mere-human about that (I think), but I can't find where that was now.

It was in this thread. Don deleted it.
You might find it in your mail.

@donho
Copy link
Member

donho commented Jun 15, 2023

@alankilborn

It is good to find the culprit, but unfortunately for me, it was a change I made (and no one else saw anything wrong with it at the time).

I've pointed out the PR as culprit, not the dev who has done the PR (and me who is the one to validate it).
Frankly, with keyword "read-only" which matched the argument, it seemed the PR makes more sense...

So, really then, my change logically was reasonable, but it exposed a different problem; I saw some question Don asked of mere-human about that (I think), but I can't find where that was now.

It was about the code before your modification. I don't understand why the 2nd argument isRecursive of method doOpen used the value of flg.isReadOnly(). Then it turns out that it was me who has done this code since long time ago, mere-human has just reformatted the code. There could be a reason or it could be my error. Anyway, it's hard to find "why".

@alankilborn
Copy link
Contributor

@donho

I've pointed out the PR as culprit, not the dev who has done the PR

I know you weren't blaming me; but before I fully investigated I was blaming myself. :-)

@Yaron10
Copy link

Yaron10 commented Jun 15, 2023

@alankilborn,

We often blame ourselves much more severely than others blame us. :)

And now it's my turn.
Reading my "Sometimes amateurs get lucky" comment - it seems quite inappropriate and out of place.
I do apologize.

@mere-human
Copy link
Contributor

mere-human commented Jun 15, 2023

It was about the code before your modification. I don't understand why the 2nd argument isRecursive of method doOpen used the value of flg.isReadOnly(). Then it turns out that it was me who has done this code since long time ago, mere-human has just reformatted the code.

Hi @donho

Sorry for the late response. I didn't understand at first. Turns out, it was my change about CustomFileDialog that appeared in "git blame". But in reality, it preserved the old behavior added by you (as it was pointed out)
https://github.com/notepad-plus-plus/notepad-plus-plus/blame/ab5c1d3e2a9718a3cc365c1e44bfae09bb7355bc/PowerEditor/src/NppIO.cpp#L1844

Anyway, I'm glad all you guys continue to improve good old Notepad++. Keep up the good work!
I don't have time to contribute but I'll gladly respond to any comments.

@Yaron10
Copy link

Yaron10 commented Jun 16, 2023

@mere-human,

I hope you're doing well.

Are _hasReadonly (in CustomFileDialog) and its functions needed now?

Thank you.

@mere-human
Copy link
Contributor

mere-human commented Jun 16, 2023

Are _hasReadonly (in CustomFileDialog) and its functions needed now?

@Yaron10 ,

As far as I can see, CustomFileDialog::isReadonly() which returns _hasReadonly, isn't currently used.
_hasReadonly is set to true only if a selected item (file or folder) has SFGAO_READONLY attribute:
https://learn.microsoft.com/en-us/windows/win32/shell/sfgao

So, you can keep it in case somebody needs it in the future. Or remove at all.

@Yaron10
Copy link

Yaron10 commented Jun 16, 2023

@mere-human,

Thank you. 👍

If and when you have some time: #13790.

donho added a commit to donho/notepad-plus-plus that referenced this issue Jun 17, 2023
@cs-hv
Copy link
Author

cs-hv commented Jun 21, 2023

Happy to see the issue could be reproduced and fixed.
Thank you all for your great work!

@cs-hv
Copy link
Author

cs-hv commented Jul 17, 2024

Sorry guys for re-activating this old thread but since some versions there is a new regression which can be reproduced with e.g. Notepad++ 8.6.8 (but also some earlier versions).
After removing the read-only flag (even from Notepad++'s menu) editing the file is possible but Ctrl-V (paste something) is not possible as initial editing command. At least a cursor movement is required but this way you will loose e.g. the mark of an already marked text area.

donho pushed a commit that referenced this issue Jul 20, 2024
Syncronize Paste and Undo commands with file buffer read-only status.
Previously, after e.g. removing the read-only flag, file editing was possible immediately, but Ctrl-V was not. At least a cursor movement or switching between the N++ tabs or apps-switching was required (SCN_UPDATEUI message generated...).

Fix #13742 (comment), close #15452
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants