shriprem / FWDataViz

Fixed Width Data Visualizer plugin for Notepad++. Turns Notepad++ into Excel for fixed-width data files. Displays cursor position data. Jumps to specific fields. Folding Record Blocks. Extracts Data. Builtin dialogs to configure file-type, record-type & fields; Themes & Colors; and Folding. Handles homogenous, mixed & multi-line records.
GNU General Public License v2.0
37 stars 6 forks source link

"FWDataViz.dll is not compatible with the current version of Notepad++" message in Notepad++ #47

Closed shriprem closed 2 years ago

shriprem commented 2 years ago

From @Yaron10's comment in the Fully implemented - Don Ho's version of Preference dialog pull request.

I've tried to install v2.4.0.2 via PluginsAdmin and also manually, but I get the following message: Clip NPP 8.3.3. I've tried both x32 and x64.

shriprem commented 2 years ago

@Yaron10, thank you for your interest in testing out this plugin and reporting this issue.

I am unable to replicate this issue with NPP 8.3.3 and NPP 8.4 RC2 in x32 and x64 combinations.

I wonder if there is a conflict arising from one of the other plugins in your NPP installation. Would you mind posting the Debug Info from Notepad++, please?

Also, if possible, try downloading FWDataViz plugin using a totally clean (i.e., no other external plugins) portable version of NPP (8.3.3 or 8.4 RC2 - doesn't matter). If the RTL setup you have is at play in this issue, it would manifest in a totally clean portable version as well.

Thanks again for your time.

Yaron10 commented 2 years ago

Hello @shriprem,

First, I'd like to thank you again for implementing the Frame feature. I was impressed by your common sense and thoroughness.


I've downloaded a fresh NPP portable v.8.4, installed FWDataViz via PluginsAdmin and had the same message. It is bewildering. :)

Notepad++ v8.4 (64-bit) Build time : Apr 20 2022 - 03:31:06 Path : C:---\npp.8.4.portable.x64\notepad++.exe Command Line : Admin mode : ON Local Conf mode : ON Cloud Config : OFF OS Name : Windows 10 Pro (64-bit) OS Version : 2009 OS Build : 19042.985 Current ANSI codepage : 1255 Plugins : mimeTools.dll NppConverter.dll NppExport.dll

shriprem commented 2 years ago

@Yaron10, I too can produce the message box that you got but only with some deliberate manipulation.

With the 2.4.0.0 release of the plugin, I split off the dark mode rendering functionality for the plugin to a separate NPP_Plugin_Darkmode.dll. I had mentioned this in the release notes:

Darkmode for the FWDataViz plugin is now rendered by a standalone DLL that can be shared by multiple plugins, thus reducing the overall memory footprint. Notepad++ will only load a copy of the NPP_Plugin_Darkmode.dll located either in:

The folder for the first plugin (in alphabetical order) that requires this DLL. Or, in the common <NPP_executable_folder>.

So, starting with the 2.4.0.0 release, both FWDataviz.dll and NPP_Plugin_Darkmode.dll are shipped in the plugin download package (zip file). With the FWDataViz plugin properly installed, you should find both these DLLs in the <NPP_Plugins_folder>/FWDataviz folder.

If I were to either delete the NPP_Plugin_Darkmode.dll file or rename it, say, by adding an 'x' at the end of the file name, and then open Notepad++, I get this message: image

This is identical to the message box you have been getting consistently. It should be noted that although a dependent DLL was the one actually missing, the NPP message just indicates the error is with the FWDataviz.dll.

But I am sure you are not deliberately deleting or renaming the NPP_Plugin_Darkmode.dll file. I am also sure that you got this DLL in the <NPP_Plugins_folder>/FWDataviz when you downloaded the plugin. But please confirm it by checking for this DLL in the <NPP_Plugins_folder>/FWDataviz folder.

While compiling the FWDataViz plugin, NPP_Plugin_Darkmode.lib is specified as an additional dependency for the linker. Here are the other dependencies specified along with it in the VS Project settings:

shlwapi.lib
kernel32.lib
user32.lib
gdi32.lib
winspool.lib
comdlg32.lib
advapi32.lib
shell32.lib
ole32.lib
oleaut32.lib
uuid.lib
odbc32.lib
odbccp32.lib
version.lib

I suspect that any one of these standard library includes could be causing the plugin load failure in your case. If so, at this moment, I am not sure how I can resolve it. Do you have any suggestions? Thanks!

shriprem commented 2 years ago

@Yaron10, only the NPP_Plugin_Darkmode.dll was being dynamically linked in during the FWDataviz.dll compilation. OTOH, the standard libraries are statically linked in, and so are part of the FWDataviz.dll. Therefore, the error message you saw is most likely from the non-availability of the NPP_Plugin_Darkmode.dll during the plugin load process of NPP.

Just to confirm if that is indeed the case, I have compiled a special version of the plugin DLL with the NPP_Plugin_Darkmode.dll also statically linked in. This 64-bit version of the plugin DLL does NOT need the NPP_Plugin_Darkmode.dll anymore. Please find it in the zip attachment: FWDataViz_x64_standalone.zip.

Please extract this DLL from the zip file into the <NPP_Plugins_folder>/FWDataviz folder of your 64-bit NPP. Try launching NPP.

If this plugin DLL is able to load fine, then these intriguing [at least for me] questions will remain to be answered:

  1. What is preventing the NPP_Plugin_Darkmode.dll from loading together with the 2.4.0.2 version of the FWDataviz.dll on your PC?
  2. And why has it not been an issue with at least 3.5K users of this plugin since December 2021 when this paired loading design was first introduced?

Again, I appreciate your time.

Yaron10 commented 2 years ago

@shriprem,

The "special version of the plugin DLL" loads without a problem. 👍

What is preventing the NPP_Plugin_Darkmode.dll from loading together with the 2.4.0.2 version of the FWDataviz.dll on your PC? And why has it not been an issue with at least 3.5K users of this plugin since December 2021 when this paired loading design was first introduced?

Very good points. The only thing I can think of is some issue/setting in my system. Please see NPP does not auto-restart after installing plugins (64-bit only).


But please confirm it by checking for this DLL in the /FWDataviz folder.

Yes, both DLLs are in the FWDataviz folder.

I suspect that any one of these standard library includes could be causing the plugin load failure in your case. If so, at this moment, I am not sure how I can resolve it. Do you have any suggestions?

But, as you've pointed out, it does work for you and thousands of other users.


Now that I've been able to use the plugin, the star was certainly justified. :) It seems you've put a lot of work into it, and the result is indeed very impressive. 👍 (Naturally, I haven't tested all the various features).

Thank you for your work. If you think there's anything else I can try, please let me know.

Best regards.

shriprem commented 2 years ago

@Yaron10, I appreciate your very diligent feedback and the time you took to test out my suggestions. As a coder, I care much more for feedbacks -- both good & bad -- than compliments or praise. And when I get them, my heart swells with satisfaction, making my efforts worthwhile. So, thank you!

It seems you've put a lot of work into it, and the result is indeed very impressive. 👍

The motivation for this plugin came from the needs at my day job where I have to deal with fixed width files in 7 different formats. And most of those files originate from outside sources, and are sent in for processing in our system. While reviewing such files to identify processing issues, I had to have a printed copy of the file layouts by my side. So, this plugin was my way of getting out of that annoyance.

The only thing I can think of is some issue/setting in my system. Please see https://github.com/notepad-plus-plus/notepad-plus-plus/issues/11435.

Until yesterday, I had a vague notion of static and dynamic linking. After the time spent looking into your issue, I now have a slightly better idea of the concept. With that caveat, I think there could be some issues with how NPP installs & loads plugins. I will look into this part of NPP code -- just out of curiosity to see if anything is amiss.

With the next release of FWDataViz, I was actually planning to bring the dark mode rendering code back into the plugin DLL. This is because I will be able to use the new NPPM_GETDARKMODECOLORS API, currently an accepted PR for NPP. With that API, there will be no need for using the TinyXML library to parse NPP's config.xml file for the currently selected dark mode tone setting & the custom tone dark mode colors. So, achieving dark mode rendering for any NPP plugin will be just a few extra classes, and not worth a separate DLL.

My next planned task is to put in a PR for this command line parameters issue. I already have the PR code ready but need to go through the issue creation & PR submission process at the NPP repo.

Yaron10 commented 2 years ago

@shriprem,

Thank you for your kind words. You are a gentleman. I'm glad I've been able to reciprocate a bit. :)

I think there could be some issues with how NPP installs & loads plugins. I will look into this part of NPP code -- just out of curiosity to see if anything is amiss.

I would highly appreciate that.

This is because I will be able to use the new NPPM_GETDARKMODECOLORS API...

I don't use Dark Mode and I've excluded all the relevant code from my NPP build. (When I report an issue, I make sure to test it with the original NPP too). But I'm glad the new API helps you.

My next planned task is to put in a PR for this https://github.com/shriprem/Goto-Line-Col-NPP-Plugin/issues/11#issuecomment-936863058.

I hardly use the command line in NPP. But again, I wish you luck with that too.


I've encountered a minor issue with the new Frame. With your permission, I'll outline it briefly here. Please let me know if you'd rather discuss it here and suggest a baked solution.

If Frame is selected, you get a frame in the Search Results too. For my personal use I'd rather have a Frame in the editor and a Full Line in the Search Results.

There are 2 options:

  1. Add separate settings for the Search Results.
  2. Keep the old behavior in the Search Results (i.e. Full Line) regardless of the editor's preference.

I think the second option should be better at this stage. What do you think?

Note: _scintView.execute(SCI_SETCARETLINEVISIBLE, 1); is still used in FindReplaceDlg.cpp.

Thank you.

Yaron10 commented 2 years ago

I've just realized https://github.com/shriprem/Goto-Line-Col-NPP-Plugin/issues/11#issuecomment-936863058 was an issue in another plugin of yours. :)

👍 I'll try it later.

shriprem commented 2 years ago

Thanks @Yaron10.

Note: _scintView.execute(SCI_SETCARETLINEVISIBLE, 1); is still used in FindReplaceDlg.cpp.

In the PR for fixing issue #11433 in NPP, I only focused on fixing that issue you found in NPP. There are quite a few places in NPP where deprecated Scintilla calls are in use. Trying to fix all those instances may have opened a can of worms. So, I stayed away from them to expedite the fix for 11433.

For the other issues you have noted, I need some time to review the relevant NPP code.

shriprem commented 2 years ago

I've just realized https://github.com/shriprem/Goto-Line-Col-NPP-Plugin/issues/11#issuecomment-936863058 was an issue in another plugin of yours. :)

I'll try it later.

@Yaron10, I expect that you will run into the same NPP_Plugin_Darkmode.dll loading issue on that as well. I am planning to reintegrate the dark mode render code back into the plugin DLL for that as well for the next release. But if you need a testable version now, please let me know. Thanks!

Yaron10 commented 2 years ago

In the PR for fixing issue #11433 in NPP, I only focused on fixing that issue you found in NPP. There are quite a few places in NPP where deprecated Scintilla calls are in use. Trying to fix all those instances may have opened a can of worms.

:+1:

Replacing https://github.com/notepad-plus-plus/notepad-plus-plus/blob/292c057f655ec5e691d92045ab78c24c01b8b102/PowerEditor/src/ScintillaComponent/FindReplaceDlg.cpp#L4332 with

            _scintView.execute(SCI_SETELEMENTCOLOUR, SC_ELEMENT_CARET_LINE_BACK, pStyle->_bgColor);
            _scintView.execute(SCI_SETCARETLINEFRAME, 0);
            _scintView.execute(SCI_SETCARETLINEVISIBLEALWAYS, true);

solves the issue (the simpler approach). You can then remove SCI_SETCARETLINEVISIBLE and SCI_SETCARETLINEVISIBLEALWAYS elsewhere in that file.

Could you please test it? Thank you.


I've just manually installed the official FWDataViz on Windows 7 x32. I got the following message:

תמונה

Translating the Hebrew words to English: Access Violation.

shriprem commented 2 years ago

Could you please test it?

It works !👍.

But now Don has to agree with you that all users who would prefer to have a frame in the main & secondary edit controls will also prefer a full highlight in the Search Results control. 😄


Translating the Hebrew words to English: Access Violation.

Interesting. So you mean to say you were getting "The specified module could not be found" with the x64 version, and now you are getting the "Access violation" message with the x32 version of FWDataviz.dll (that also needs a separate NPP_Plugin_Darkmode.dll to load with it)?

The other difference I see between the x64 and x32 message boxes is that in the x32 message box, the text and buttons have an RTL layout.

If the issue with the x32 version has also due to the failed paired DLL loading, I have prepared the special standalone version of the x32 DLL: FWDataViz_x32_standalone.zip.

Please test this and let me know if there is still an issue with the x32 version. Thanks!

Yaron10 commented 2 years ago

Thank you for testing it. 👍

that all users who would prefer to have a frame in the main & secondary edit controls will also prefer a full highlight in the Search Results control.

I'm not assuming that.

But now Don has to agree with you...

I'm not going to argue with him again. :) But this a kind of emergency fix, and the logic is as follows: in previous versions we had a Full Line highlight in the Search Results window even if "Highlight current line" was unchecked. So this behavior is kept with my proposed solution. A more complex solution could be implemented in the future.


So you mean to say you were getting "The specified module could not be found" with the x64 version...

Sorry for not emphasizing it. I've tested both x32 and x64 on Windows 10, and got the message in the first post here. Today I've tested it on Windows 7 x32, and got the different message.

I don't have access to the Windows 7 PC at the moment. I'll test your modified DLL tomorrow.

Good night.

shriprem commented 2 years ago

in previous versions we had a Full Line highlight in the Search Results window even if "Highlight current line" was unchecked. So this behavior is kept with my proposed solution.

👍 OK. I now get the logic behind your solution.


Sorry, I missed the Windows 7 part in your prior message. Thanks for the clarification.

Good night.

Yaron10 commented 2 years ago

I've figured out the problem on Windows 7.

I've downloaded a fresh portable v8.4, installed FWDataViz via PluginsAdmin and it loaded without a problem. I then used the "fresh" notepad++.exe in my old folder, and got the error message. It took me a while to understand the culprit was the config.xml created by my NPP build.

Could you please try the following steps?

  1. Open config.xml with another editor, replace the <GUIConfig name="ToolBar" entry with <GUIConfig name="ToolBar" visible="yes" /> and save.
  2. Use the official DLL.
  3. Restart NPP.

I apologize for not testing it first with a fresh portable NPP.

I'd like to emphasize that this was not the case on my Windows 10 PC. I got the error message on that machine with a fresh portable v8.4.


in previous versions we had a Full Line highlight in the Search Results window even if "Highlight current line" was unchecked. So this behavior is kept with my proposed solution.

OK. I now get the logic behind your solution.

:+1:

I'd like to clarify it a bit more. In previous versions it was obvious that the "Highlight current line" preference does not affect the Search Results window. With your (great) Frame PR, it's not so clear: if you select "None" you get a Full Line in the Search Results, but if you select "Frame" you get a Frame.

Also - after switching from "None" to "Frame", the highlighting in the Search Results changes only after a new search.

Those are minor issue. If you think it's better to leave it as it is, I'll respect your decision.

Thank you.

shriprem commented 2 years ago

@Yaron10, thank you for that insight into the issue you were having on Windows 7. It took me quite sometime to figure out how I should have coded to handle for the config.xml tweak you had on the Windows 7 PC. But I feel it was time well spent.

These are the relevant lines from the code that is using TinyXML library to parse NPP's config.xml:

      if (guiConfig->Attribute("name", "ToolBar")) {
         string sTBType{ guiConfig->GetText() };
         ...
     }

Instead, if the 2nd line had been coded as below, it would have handled the case for an empty text node.

      if (guiConfig->Attribute("name", "ToolBar")) {
         string sTBType{ guiConfig->GetText() ? guiConfig->GetText() : "" };
         ...
     }

In any case, I will be getting rid off all this code in the next release for my plugins. Instead, I will be using NPP's new NPPM_GETDARKMODECOLORS API for querying the dark mode settings.


Those are minor issue. If you think it's better to leave it as it is, I'll respect your decision.

You have seen how many rejections and change requests I had to endure to get the Framed Line feature accepted for NPP. If we start adding the issues with the Search Results control into the mix now, I fear they may develop cold feet and withdraw the 'Accepted' status for the whole PR. So, I feel that we should just get this PR merged for the next NPP release. The benefits of getting it in outweigh not doing so.

After that NPP release, please feel free to raise these issues. I will be glad to offer my assistance for getting those issues fixed.

shriprem commented 2 years ago

You may already know this. But this is something I hadn't noticed before in NPP. This may explain why the Search Results control is ignoring the 'None' setting for the Current Line Highlight, and handling it the same as 'Full Highlight'.

The 'Search result' has its own style configuration in NPP. image

Even if the user may have selected 'None' for the current line highlight, NPP is treating the "Current line background" color for the 'Search result' as the overriding preference, and rendering the Search Results control with full highlight. If the user needs the equivalent of the 'None' for the current line highlight, they will have to set the current line background for 'Search result' to be the same as the global background color. But you may have already known this.

Yaron10 commented 2 years ago

Instead, if the 2nd line had been coded as below... ... In any case, I will be getting rid off all this code in the next release for my plugins.

👍 I'm watching your repo now, and I'll test the new version. :) Anyway, your code was OK. As I've mentioned in your PR, NPP can crash after manually modifying config.xml.


You may already know this. But this is something I hadn't noticed before in NPP. This may explain why the Search Results control is ignoring the 'None' setting for the Current Line Highlight, and handling it the same as 'Full Highlight'.

Yes, I was aware of that style. NPP highlights the current line in the Search Results regardless of the preference. https://github.com/notepad-plus-plus/notepad-plus-plus/blob/292c057f655ec5e691d92045ab78c24c01b8b102/PowerEditor/src/ScintillaComponent/FindReplaceDlg.cpp#L4332.

You have seen how many rejections and change requests I had to endure to get the Framed Line feature accepted for NPP...

Good points. You're absolutely right. And thank you for the willingness to assist if and when.

shriprem commented 2 years ago

NPP highlights the current line in the Search Results regardless of the preference.

I think that NPP is treating the current line background color for 'Search result' as having higher specificity just like how CSS handles overlapping style settings for an HTML document. A proper solution for this would be to replicate the entire Current Line Indicator groupbox & its controls for the Search result style configuration interface as well.

But this will also mean adding new attributes for the "Current line background colour" WordsStyle element in stylers.xml. I am referring to the highlighted line here: image

With all this, NPP will be able to properly cater to all users' preferences for the Search results control.


Another observation. In NppBigSitch.cpp, by commenting out the line for the _subEditView as shown below, I gained a bit of insight into the implementation of Scintilla.

        case NPPM_INTERNAL_CARETLINEFRAME:
        {
            _mainEditView.execute(SCI_SETCARETLINEFRAME, lParam);
            //_subEditView.execute(SCI_SETCARETLINEFRAME, lParam);
            return TRUE;
        }

With NPP executable built with this commented line:

  1. Launch NPP. Have at least two files open.
  2. Choose 'Frame' as the Current Line Indicator. Change the value for the frame width.
  3. Only after step 2, move a file to the secondary view in NPP. You will note that this secondary view (i.e., the _subEditView) also opened with the same frame width that you specified in step 2. This is happening even though the line for the _subEditView is commented for setting the frame width. That is because Scintilla is using the settings of the _mainEditView as a base model to instantiate the _subEditView.
  4. Now, change the frame width to a different value in Preferences. But this time, you will notice that those changes are being applied only on the main view due to the commented line.

A similar replication of the main view settings is happening when Scintilla is instantiating the Search result control (unless different settings are specified in the code for that new Scintilla control after its instantiation). Also, if you change the frame width while the Search result control is open, those changes will not be reflected in the Search result control just as it was not reflected in the subEditView due to the commented line. Hope I am making sense.

Yaron10 commented 2 years ago

A proper solution for this would be to replicate the entire Current Line Indicator groupbox & its controls for the Search result style configuration interface as well.

Yes, that is the proper and more complex solution. Do you mean placing it in Style Configurator? Wouldn't the Searching tab in Preferences be more appropriate?

But this will also mean adding new attributes for the "Current line background colour" WordsStyle element in stylers.xml.

I'm not sure I understand it. Why not handle it like the current controls and add new entries to config.xml?

Another observation...

👍 Very interesting.

Thank you.

Yaron10 commented 2 years ago

Wouldn't the Searching tab in Preferences be more appropriate?

Another option could be keeping it where it is and adding 2 horizontal CheckBoxes: "Editor" and "Search Results". And a third option: the Search Results context-menu. What do you think?

shriprem commented 2 years ago

Or, we could keep it simple, and just have the full highlight only for the Search results control. That will involve just the 2 lines of code you had already provided for your emergency fix. It will suffice until another user wants the current line in that control in a different way. But I doubt it since not many are as sharp-eyed as you are. 😄

Wouldn't the Searching tab in Preferences be more appropriate?

Another option could be keeping it where it is and adding 2 horizontal CheckBoxes: "Editor" and "Search Results". And a third option: the Search Results context-menu. What do you think?

My line of thinking was that having two such set of controls together on the same dialog interface would cause too much clutter and overwhelm many users' comprehension. And my line of thought was also influenced by...

But this will also mean adding new attributes for the "Current line background colour" WordsStyle element in stylers.xml.

I'm not sure I understand it. Why not handle it like the current controls and add new entries to config.xml?

My line of thought was influenced by CSS and its specificities. I will reiterate what I was saying earlier that the Current line background color for the Search result is having a higher specificity since it is a more local-specific preference than the None/Highlight values in Preferences - where the preferences are at a global level.

In the same vein, the None/Highlight/Frame preference for the Search result control would also be more local-specific. So, keep it together with where the other local-specific preferences for the Search result controls are being specified. And, store those preference choices together in Styler.xml.

Just two different ways of reasoning -- mine and yours. I really don't use the Search result control in NPP that much. So, whatever way is adopted to address your preferences for the Search result control, I will be totally fine with it. Don will be the final arbiter.

Yaron10 commented 2 years ago

Sorry for the late response. I wasn't by my PC.

Your Frame PR has been merged. Congrats and thanks again.

Or, we could keep it simple, and just have the full highlight only for the Search results control. That will involve just the 2 lines of code you had already provided for your https://github.com/shriprem/FWDataViz/issues/47#issuecomment-1107911653. It will suffice until another user wants the current line in that control in a different way. But I doubt it since not many are as sharp-eyed as you are.

Thanks. :) I also think that's the best approach at this stage. Can I leave it to you? I mean if and when to submit a PR.

That makes the following lines somewhat irrelevant. Still...

My line of thinking was that having two such set of controls together on the same dialog interface would cause too much clutter and overwhelm many users' comprehension.

I agree. That was not a good idea. But the Search Results context-menu was good. :)

Regarding Styler.xml: I understand your line of thought better now. You can argue that if the FontSize is saved there, so can the highlighting pref. But there aren't any other similar prefs in Styler.xml. It only has Font and Color. Actually, my "emergency fix" is in void Finder::setFinderStyle(). Well, it's a thin line.

Good night.

shriprem commented 2 years ago

@Yaron10, very much appreciate your understanding.

Can I leave it to you? I mean if and when to submit a PR.

Please open an issue in NPP. Being a more recognized name, an issue created by you will carry more weight. Also, since the Frame PR got merged now, it is safe to tag this as a regression.

After opening the issue tag me in the issue OR alert me here. I will be happy to submit a PR for that issue. Thanks!

Yaron10 commented 2 years ago

Done.

Thank you also for your recent PRs. 👍 I liked the UI one. :) I don't use the command line frequently.

shriprem commented 2 years ago

I have pushed PR 11598 using the exact lines of code you had provided earlier. But I have two observations.

  1. re: _scintView.execute(SCI_SETCARETLINEVISIBLEALWAYS, true);. This line goes somewhat beyond what is requested in issue 11596. I hope Don overlooks that if that is what you need.
  2. re. the statement: "_You can then remove SCI_SETCARETLINEVISIBLE and SCI_SETCARETLINEVISIBLEALWAYS elsewhere in that file_." in your comment. There were no other instances where those two API calls were used in FindReplaceDlg.cpp.

Finally, I also want to point out that it is time to consider an "Advanced Options" generic interface for NPP. This generic interface will be somewhat similar to the chrome://flags/ option interface in the Chrome browser or the about:config option interface in the Firefox browser. Such interfaces are rendered by a generic engine using a list of options from a text file. There will be no need for special coding for the UI interface for any specific advanced option. That is why I call it a generic interface. With such an interface, when there is a need to include a new advanced option, only the options list needs to be updated in a text file, and the rendering engine will automatically include it in its output interface. There will be no need to worry about UI layouts, groupboxes, radio buttons, check boxes, etc.

Apart from Chrome & Firefox browsers, other applications such as VS Code & xPlorer2 have embraced this model as well. Here is a clip of the Advanced Options dialog from xPlorer2: image

Yaron10 commented 2 years ago

Thank you for submitting the PR. 👍

But I have two observations.

https://github.com/notepad-plus-plus/notepad-plus-plus/blob/292c057f655ec5e691d92045ab78c24c01b8b102/PowerEditor/src/ScintillaComponent/FindReplaceDlg.cpp#L2683 https://github.com/notepad-plus-plus/notepad-plus-plus/blob/292c057f655ec5e691d92045ab78c24c01b8b102/PowerEditor/src/ScintillaComponent/FindReplaceDlg.cpp#L2817

What am I missing? :)

Without SCI_SETCARETLINEVISIBLEALWAYS, the current line in Search Results would not be highlighted after double-clicking and moving the focus to the editor. This would be highly confusing since the caret is invisible in that window.

With your permission, I'll "thumb up" the PR after your reply. It's not that you don't deserve it now. :) I'd just rather do that after solving the misunderstanding here.

Finally, I also want to point out that it is time to consider an "Advanced Options" generic interface for NPP.

👍 I have a bookmark for AboutConfig. I think it's a good idea. Try suggesting it to Don though. :) There are a few hidden settings in NPP. But not in the AboutConfig format.

shriprem commented 2 years ago

@Yaron10, probably my NPP has some settings tweaked that I am not aware of. In my NPP v8.4, all views retain their full highlights even when not in focus. Or, probably it is my Windows 11 too. image

That is why I am unable to reproduce the issue #11595. Also, why I didn't see the need for SCI_SETCARETLINEVISIBLEALWAYS. Maybe it is needed for other users.

By the way, I brought up the need for a generic Advanced Options interface in connection to the various complicated UI options we discussed earlier for your Search results full highlight issue.

Yaron10 commented 2 years ago

Both views are set to (SCI_SETCARETLINEVISIBLEALWAYS, true). Did you find it in FindReplaceDlg.cpp?

That is why I am unable to reproduce...

This is not related to SCI_SETCARETLINEVISIBLEALWAYS. Could you try reproducing it without any plugins?

By the way, I brought up the need for a generic Advanced Options interface in connection to the various complicated we discussed earlier for your Search results full highlight issue.

Yes, I understood the context. :)

shriprem commented 2 years ago

I get full highlights in all views in pristine portable versions of NPP v8.4; with no other external plugins; both with x32 & x64; on both my Windows 11 and Windows 7 PCs. That is 4 different combos of pristine versions.

I was thinking issue #11595 was also related to a missing SCI_SETCARETLINEVISIBLEALWAYS call. Though I haven't checked the NPP code yet to confirm if this is so. But without being able to reproduce the issue on my PC, I don't want to venture a fix for that issue.

shriprem commented 2 years ago

Did you find it in FindReplaceDlg.cpp?

Yes, they are also in FindReplaceDlg::createFinder(). I found it now. Those two calls are at lines 2683 and the line in your code is at line 4334. I don't feel comfortable removing those two lines which are over 1500 lines apart from the new call. There might be implications I don't have the time to analyze. So, I didn't remove it.

Yaron10 commented 2 years ago

I get full highlights in all views in pristine portable versions of NPP v8.4; with no other external plugins; both with x32 & x64; on both my Windows 11 and Windows 7 PCs. That is 4 different combos of pristine versions.

That's expected. They are all set to (SCI_SETCARETLINEVISIBLEALWAYS, true).

Regarding 11595: Could you please try it with a fresh portable? See also: https://github.com/notepad-plus-plus/notepad-plus-plus/pull/11419#issuecomment-1094362796.

shriprem commented 2 years ago

Pristine. Fresh. Much difference?

Yaron10 commented 2 years ago

I don't feel comfortable removing those two lines...

OK. As you understand.

Pristine. Fresh. Much difference?

I thought you only tried the line highlight with a fresh portable.

Yaron10 commented 2 years ago

@shriprem,

Would you be comfortable asking Don about the lines I think should be removed? If not, would you mind if I did that? It's just a bloat.

Thank you.

shriprem commented 2 years ago

Not really much of a bloat since those extra two calls on SCI_SETCARETLINEVISIBLEALWAYS each take a few microseconds on any modern PC.

But if you feel compelled that they should be removed, here is my analysis to help your case:

Finder::setFinderStyle() is being called from 3 other functions -- all within FindReplaceDlg.cpp & FindReplaceDlg.h.

  1. FindReplaceDlg::findAllIn: On line 2684, SCI_SETCARETLINEVISIBLEALWAYS is being called. Then a bit later on line 2716, _pFinder->setFinderStyle(); is called which again calls SCI_SETCARETLINEVISIBLEALWAYS.

  2. FindReplaceDlg::createFinder: On line 2818, SCI_SETCARETLINEVISIBLEALWAYS is being called. Then a bit later on line 2845, _pFinder->setFinderStyle(); is called which again calls SCI_SETCARETLINEVISIBLEALWAYS.

  3. In FindReplaceDlg.h, updateFinderScintilla(): On line 339, only _pFinder->setFinderStyle(); is called which calls SCI_SETCARETLINEVISIBLEALWAYS.

So, the SCI_SETCARETLINEVISIBLEALWAYS calls on 1 & 2 can be removed.

Yaron10 commented 2 years ago

Thank you for the helpful analyses. 👍 I'll comment there.

Yaron10 commented 2 years ago

@shriprem,

The latest 2.5.0 loads without an issue on my Windows 10 PC. 👍 Great work.

Thank you.

shriprem commented 2 years ago

Thank you for the feedback, @Yaron10.

shriprem commented 2 years ago

@Yaron10 , greetings!

Sorry to bother you. I am working to get an incompatibility crash issue that occurs when the CSVLint plugin is installed. I have created two issues at the CSVLint repo on this. If possible, please review the more recent one of those two. Here: https://github.com/BdR76/CSVLint/issues/26

  1. Please let me know if I, as a plugin author myself, am being fair and not disrespectful in reporting this issue with another plugin.
  2. Also, maybe you are familiar with a few other plugins implemented in C#, that also had to patch their code for the NPP 8.4 Scintilla5/Lexilla5 upgrade. If so, please share their names with me.

Thank you. Appreciate your time!

Yaron10 commented 2 years ago

Hello @shriprem,

I hope you're doing well. You're not bothering me at all. It's a pleasure.

Please let me know if I, as a plugin author myself, am being fair and not disrespectful in reporting this issue with another plugin.

I appreciate the thoughtfulness, but I don't see anything disrespectful in that.

Also, maybe you are familiar with a few other plugins implemented in C#, that also had to patch their code for the NPP 8.4 Scintilla5/Lexilla5 upgrade. If so, please share their names with me.

Unfortunately I'm not familiar with any other plugins implemented in C#. @ rdipardo (remove the space) might know.

Best regards.


Thanks again for the Frame feature. I've been using it for the past few weeks, and I really like it.

shriprem commented 2 years ago

Thank you for your very timely response, @Yaron10.

After messaging you, I later found this very relevant post: https://community.notepad-plus-plus.org/post/76447

Therein Ekopalypse mentions in response to a query from CSVLint's author that these additional methods may need to be implemented for iLexer5:

class ILexer5 : public ILexer4 {
public:
        virtual const char * SCI_METHOD GetName() = 0;
        virtual int SCI_METHOD  GetIdentifier() = 0;
        virtual const char * SCI_METHOD PropertyGet(const char *key) = 0;
};

Of these, the GetIdentifier() function is already implemented in the CSVLint source code. Therefore, when I execute editor.getIdentifier() with the CSVLint lexer active, there is no crash.

But the GetName() and PropertyGet() methods are missing in CSVLint code. Correspondingly, therefore, both editor.getLexerLanguage() and editor.getProperty('TESTING') are crashing NPP. Without knowing all these details, I have somehow discovered these two bugs with CSVLint.

I am not at all familiar with C#. But as a personal challenge, I am planning to implement these two missing methods in my local repo of CSVLint and see if it fixes the crash issues. I will keep you posted. Thanks for your time.

Yaron10 commented 2 years ago

My pleasure.

Good luck with fixing those bugs. If and when you do, you might want to update the community for other users of that plugin (I've never tried it myself).

shriprem commented 2 years ago

@Yaron10, after a bit of struggle, I did manage to provide a fix for the CSVLint plugin in this PR: https://github.com/BdR76/CSVLint/pull/27.

Thanks to your suggestion, I have also noted these requirements for lexer plugins at: https://community.notepad-plus-plus.org/post/77164. Cheers!

Yaron10 commented 2 years ago

@shriprem,

I'm glad you've manage to fix the issue. 👍