pnedev / comparePlus

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

Some "Equalizer" issues #298

Closed Yaron10 closed 2 years ago

Yaron10 commented 2 years ago

Hello Pavel,

Could you please try the following case?

STR: Compare these files. Place the caret on line 621 in FindReplaceDlgOld.cpp. Mouse-scroll down to line 631 in FindReplaceDlgOld.cpp and make sure that line 760 in FindReplaceDlgNew.cpp is visible.

תמונה

Ctrl+left-click on - in line 631.

Result:

תמונה

NOTE: If you place the caret on line 631 before Ctrl+left-clicking, the result is OK.


Personally, I'd like the caret to be moved to the line whose symbol I Ctrl+left-click. I consider the "Equalizer" as selecting and pasting. In pasting the caret would be at the pasted text end. Still, I think it would be nicer if it was placed at the symbol-line.

What do you think?

Thank you.

BR

Yaron10 commented 2 years ago

EDIT.

Ctrl+left-click on - in line 631.

NOTE: If you place the caret on line 631 before Ctrl+left-clicking, the result is OK.

pnedev commented 2 years ago

Hello Yaron,

Thanks for the example files and the description. I'll try that and write back. I won't have time this weekend so maybe next week, thanks.

BR

Yaron10 commented 2 years ago

Hello Pavel,

Thank you for looking into it, and best of luck. Whenever you have time, and according to your priority.

BR

pnedev commented 2 years ago

Hi Yaron,

One question: Do you think it is better to position the cursor on the clicked line always or only in case the Following Caret is enabled? I intentionally made the cursor to maintain its position on equalize action but in some cases the view is not properly restored as you noticed - I'll fix it soon. I was just wondering about the caret.

Thanks

Yaron10 commented 2 years ago

Hello Pavel,

or only in case the Following Caret is enabled?

I thought about it when suggesting to move the caret. It seems like selecting and pasting - caret should be moved regardless of Following Caret. But I'd certainly it if you argued the cases were not quite the same, and users who uncheck Following Caret would prefer not to move the caret on equalize.

but in some cases the view is not properly restored as you noticed - I'll fix it soon.

👍 Great! I think this is very important.

Thanks again for your work. Much appreciated.

BR

pnedev commented 2 years ago

Hello again, Yaron,

Could you try with the latest dev branch commit (https://github.com/pnedev/compare-plugin/commit/8da6ae0e635aea7e4d97f62cfc55558174610539) ? If Move caret on navigation is enabled the caret will be set to the beginning of the equalized diff. The view is now properly restored in both cases (with or without following caret). Auto-recompare must be ON - the problem was manifested on re-compare.

Tell me if you still have concerns or suggestions, thank you very much!

BR

Yaron10 commented 2 years ago

Hello Pavel,

I had to take a break.

The main issue is fixed. 👍 It reminds me of your "miracles" some years ago. Well, I know it's actually hard work. :)

The caret position is also great. Could you please try the following case?

Compare the Find files. Scroll to line 651 in FindReplaceDlgOld.cpp. Make sure the caret is above that line.

תמונה

Ctrl+Shift+Click on - in line 651.

Expected: As I see it as selecting and pasting (or deleting in this case), the caret should be in the if (lno > max_lno) lno = min_lno; line.

I'm sure a lot of users will love the "Equalizer". Thank you very much. I do appreciate it.

BR

pnedev commented 2 years ago

Hello Yaron,

That is a great catch, excellent! :+1:

Please try again the dev branch commit https://github.com/pnedev/compare-plugin/commit/5b0ed68cfe72282fea2ee411415a2c555ee23211 when you have time.

Thank you and BR

Yaron10 commented 2 years ago

Hello Pavel,

Catching is easier than fixing. And your fix is excellent. 👍

Thank you for that too.

BR

pnedev commented 2 years ago

Hi Yaron,

Could you please describe here the minor issue you have come across as you mention in https://github.com/pnedev/comparePlus/issues/307#issuecomment-1202304716 ?

Thank you :+1:

Yaron10 commented 2 years ago

Hello Pavel,

I've encountered a case where the views jump to the wrong position after using the Equalizer. Unfortunately I don't remember the files, and I'll have to do some tests. I'm exhausted now and, with your permission, I'll do that tomorrow.

Thank you.

pnedev commented 2 years ago

Hello Yaron,

OK, when you have time and energy, thank you.

Yaron10 commented 2 years ago

Hello Pavel,

Could you please try the following case?

STR: Compare these files. Go to line 5172 in FindReplaceDlgOld.cpp. Ctrl+Left-Click.

תמונה

Result: The inserted block is not visible.

I assume this and the related issues happen even if you call MakeSureLineIsVisible() (I can't remember now the exact function name).

Thank you again for you time and work. I do appreciate it. 👍


BTW, should Ctrl+Left-Click work in line 5172?

pnedev commented 2 years ago

Hello Yaron,

As I wrote in https://github.com/pnedev/comparePlus/issues/307#issuecomment-1204104305 both issues had the same root cause and they should be fixed in https://ci.appveyor.com/project/pnedev/compareplus/builds/44363424 .

should Ctrl+Left-Click work in line 5172 ?

Strange as it seems it is expected to work that way. This is because the "blank" area after 5172 is considered internally in Scintilla as the line 5172 itself (line annotation is part of the line and cannot exist separately) and if you click on the "blank" (annotation) margin you actually get a click on the line margin itself. To illustrate this just try setting a bookmark by clicking on the "blank" margin. Unfortunately there is no reasonable way to circumvent this.

Thank you.

Yaron10 commented 2 years ago

Hello Pavel,

👍 Excellent! Thank you for the quick fix. Much appreciated.

Strange as it seems it is expected to work that way. ... Unfortunately there is no reasonable way to circumvent this.

👍

Thanks for that as well.