pnedev / comparePlus

Compare plugin for Notepad++
GNU General Public License v3.0
993 stars 139 forks source link

Crashes after done comparing #353

Closed sovalyeler closed 1 year ago

sovalyeler commented 1 year ago

Hi. Thanks for this plugin. I've been using old compare plugin for a while. Today, I've discovered Compare-plus, installed it; however, it just crashes after finishing comparing two text files. I'm on NPP v8.5.2 Note: The issue is present on v8.5.1 also.

pnedev commented 1 year ago

Hi,

Can you please share a little more details? How did you install ComparePlus? What is its version? Do you have other plugins installed? Could you try using PluginsAdmin Notepad++ window (from the Plugins menu) and remove your installed Compare and ComparePlus plugins and then install again ComparePlus via PluginsAdmin? You could also try temporarily remove all other plugins as well.

Is it possible for you to share the files you are comparing (if those are not confidential) or maybe just try comparing other files and tell me if Notepad++ crashes again?

Thanks, BR

sovalyeler commented 1 year ago

I'm using the latest version of compareplus. I've disabled all plugins other than compareplus and enabled them one by one. It turned out that compareplus is incompatible w/ TextFX2 plugin.( https://github.com/rainman74/NPPTextFX2 ) Can you install TextFX2 and test it? Thank you.

pnedev commented 1 year ago

I'll see what I can do but it might be a bug in TextFX2 plugin and I'll need to investigate its code to find the root cause. It is not something I am very enthusiastic about.

sovalyeler commented 1 year ago

I understand. Thanks for your efforts. I might contact TextFX2 developer and see what he/she can do.

sovalyeler commented 1 year ago

Response from TextFX2 developer:

https://github.com/rainman74/NPPTextFX2/issues/14#issuecomment-1510508854

Yes, I can confirm the behavior (tested x64 build only). But with the normal ComparePlugin there is no problem. Therefore I refrain from an error analysis in TextFX2. If this unexpectedly requires a fix in TextFX2, feel free to post a pull request.

pnedev commented 1 year ago

I will check the issue when I get to this, thanks. BR

pnedev commented 1 year ago

I use Notepad++'s status bar space in ComparePlus (to display compare info) by intercepting Notepad++ window notifications procedure and it appears that TextFX2 does that as well. This leads to wrong handling and restoration of the original Notepad++ window procedure leading to crashes and/or undefined behavior.

I have made ComparePlus to no longer intercept and change Notepad++ window notifications procedure (as it seems to be a potential inter-plugin vulnerability). This fix will be available in the next plugin release but until then you could use the current dev build: win32, win64

BR

sovalyeler commented 1 year ago

Thanks a lot, no crashes has happened so far.

Yaron10 commented 1 year ago

Hello Pavel,

Thank you very much for this fix as well. 👍

May I ask if the old status-bar info-toggling is more complex with the new implementation? (Just curious; the new behavior is perfectly fine).

Do you think it's better to display Compare Options & Compare Summery in two separate commands?

BR

pnedev commented 1 year ago

Hello Yaron,

May I ask if the old status-bar info-toggling is more complex with the new implementation?

Since I stopped intercepting the Notepad++ window procedure I am no longer able to intercept the user click action on the status bar. That's why I implemented the new Active Compare Options command. I intend to make a setting that selects what info to display in the status bar - summary of diffs or compare options.

Do you think it's better to display Compare Options & Compare Summery in two separate commands?

I don't know. Maybe its better to have one command displaying the full info. What do you think? ... Update - I have combined both commands info into Active Compare Summary. Good suggestion, thanks :+1:

Thanks. BR

Yaron10 commented 1 year ago

Hello Pavel,

Since I stopped intercepting the Notepad++ window procedure I am no longer able to intercept the user click action...

Thank you for the explanation.

Update - I have combined both commands info into Active Compare Summary.

👍 Great. Thank you for that too.

BR

Yaron10 commented 1 year ago

Hello again Pavel,

I intend to make a setting that selects what info to display in the status bar - summary of diffs or compare options.

Are you planning to add that setting in the Settings dialog or in the menu?

In Settings you can have a ComboBox with None, Diffs and Options. In the menu you can have Toggle Status Bar Info (Diffs/Options). - And the user would still be able to quickly toggle the info by adding a toolbar button (via CustomizeToolbar) or by adding a shortcut.

What do you think?

Thank you. BR

pnedev commented 1 year ago

Hello Yaron,

I already implemented it as a setting. It is better to keep the plugin menu as short as possible and leave there only the commands and the most commonly accessible functions/configs. Please check the dev binaries x86 or x64.

Thank you, BR

Yaron10 commented 1 year ago

Hello Pavel,

My timing wasn't the best. :)

It's beautiful! 👍 Thank you very much.

If you select Disabled and re-Compare, it takes effect only after switching to a non-Compare tab and back. Is that intentional?

BR

pnedev commented 1 year ago

Hello Yaron,

It is not intentional but in order for the status bar to be updated Notepad++ needs to reload it. I'll check if I can force that from CP but more likely I'll leave it this way. Thank you.

BR

Yaron10 commented 1 year ago

Hello Pavel,

It is not intentional but in order for the status bar to be updated Notepad++ needs to reload it.

I thought about that after posting. :)

more likely I'll leave it this way.

👍 Yes, it's not worth the extra work/code.

Thanks again. I appreciate it.

BR