Hiccups while typing (continuation)

[Pecan]

@ ollydbg

I stopped overthinking this, simplified, and came up this the 7th inning. I think this is a homerun.
Give it a try. Thanks.

Code

Index: classbrowser.cpp
===================================================================
--- classbrowser.cpp	(revision 13611)
+++ classbrowser.cpp	(working copy)
@@ -46,7 +46,7 @@
 
 #include "parser/ccdebuginfo.h"
 
-#include <stack>
+//unused #include <stack>
 #include <chrono>
 
 #define CC_CLASS_BROWSER_DEBUG_OUTPUT 0
@@ -242,6 +242,9 @@
             filter = bdfProject;
 
         m_Parser->ClassBrowserOptions().displayFilter = filter;
+        // Update stale CB globals in TempParser before WriteOptions() //(ph 2025/02/10)
+        if (m_Parser == m_ParseManager->GetTempParser())
+            m_Parser->ReadOptions();
         m_Parser->WriteOptions();
         UpdateClassBrowserView();
     }
Index: parsemanager.h
===================================================================
--- parsemanager.h	(revision 13611)
+++ parsemanager.h	(working copy)
@@ -9,8 +9,8 @@
 #include "parsemanager_base.h"
 #include "parser/parser.h"
 
-#include <queue>
-#include <map>
+//unused #include <queue>
+// unused #include <map>
 #include <memory>
 #include <unordered_map>
 
@@ -257,6 +257,7 @@
     //(ph 2024/01/25)
     void SetSymbolsWindowHasFocus(bool trueOrFalse){ m_SymbolsWindowHasFocus = trueOrFalse;}
     bool GetSymbolsWindowHasFocus(){return m_SymbolsWindowHasFocus;}
+    ParserBase* GetTempParser(){return m_TempParser;} //(ph 2025/02/10)
 
 protected:
     /** When a Parser is created, we need a full parsing stage including:
Index: resources/manifest.xml
===================================================================
--- resources/manifest.xml	(revision 13611)
+++ resources/manifest.xml	(working copy)
@@ -3,7 +3,7 @@
     <SdkVersion major="1" minor="10" release="0" />
     <Plugin name="CodeCompletion">
         <Value title="Code completion" />
-        <Value version="1.0.5 24/01/29" />
+        <Value version="1.0.6 25/02/10" />
         <Value description="This plugin provides a symbols browser for your projects and code-completion inside the editor.
 
 

[ollydbg]

Hi, Pecan, thanks for your hard work!
I debugged your patch yesterday. I think this patch is simple, but it still has some errors. Let me describe the issue again. I think in our code, we just "write" or "read" from the .conf file too much. I think every Parser instance's option is valid, so I don't think there is some "stale" option.

The problem is that we have several Parser instances. For example, we have ParserA, ParserB, ParserTemp. When there is no cbp opened, the ParserTemp always exists. I think when every Parser instance get constructed, it will "read" the options from the .conf file, that's OK.

Code

                                  +----------+     
         +------------------------|Parser A  |     
         |                        +----------+     
 +-------|------+                                  
 |Parse option  |                 +----------+     
 |Browser option|-----------------|Parser B  |     
 +-------|------+                 +----------+     
         |                                         
         |                        +-----------+    
         +------------------------|Parser Temp|    
                                  +-----------+    

1, When the Parser object get constructed, all the option should be copied from the .conf file to the object.

2, When the user open a editor->CodeCompletion option dialog, and edit some options, and click the "OK" button (in the OnApply function), I think all the options will be saved.

3, When we switch the Parser object(for example, the active project is switched from A to B, or from A to Temp), when switching, we don't need to "read all the options from the .conf file", because different Parser maintain its own option.

4, When user tweak the ClassBrowser option, I think it should save to the .conf file immediately after changing the ClassBrowser option.

5, When a Parser get closed, I think we don't need to "save" its option to the .conf file", because those options were either saved in the above "2" and "4" steps.

[Pecan]

@ ollydbg

Thanks for the clarity of that last message.
Here's the result of the re-work. For CB global settings:
1) write changes to .conf when user makes a change.
2) Do not write CB globals when closing a single file or when a parser is closing.
3) The TempParser must be updated when all parsers are closed else it writes its stale global settings (when CodeCompletion plugin initialized) to the .conf .

Code

Index: ccoptionsdlg.cpp
===================================================================
--- ccoptionsdlg.cpp	(revision 13611)
+++ ccoptionsdlg.cpp	(working copy)
@@ -183,6 +183,12 @@
 
 void CCOptionsDlg::OnApply()
 {
+    cbProject* pProject = Manager::Get()->GetProjectManager()->GetActiveProject();
+     // Remember the project that changed the .conf data //(ph 2025/02/04)
+    m_ParseManager->SetOptsChangedByProject(pProject);
+    // Renember the Parser that changed the .conf data //(ph 2025/02/04)
+    m_ParseManager->SetOptsChangedByParser(&(m_ParseManager->GetParser())); //(ph 2025/02/07)
+
     ConfigManager* cfg = Manager::Get()->GetConfigManager(_T("code_completion"));
 
     // -----------------------------------------------------------------------
Index: parsemanager.cpp
===================================================================
--- parsemanager.cpp	(revision 13611)
+++ parsemanager.cpp	(working copy)
@@ -623,6 +623,7 @@
         // if the active parser is deleted, set the active parser to nullptr
         if (it->second == m_Parser)
         {
+            SetClosingParser(m_Parser); //(ph 2025/02/12)
             m_Parser = nullptr;
             SetParser(m_TempParser); // Also updates class browser
         }
Index: parsemanager.h
===================================================================
--- parsemanager.h	(revision 13611)
+++ parsemanager.h	(working copy)
@@ -9,8 +9,8 @@
 #include "parsemanager_base.h"
 #include "parser/parser.h"
 
-#include <queue>
-#include <map>
+//unused #include <queue>
+// unused #include <map>
 #include <memory>
 #include <unordered_map>
 
@@ -258,6 +258,16 @@
     void SetSymbolsWindowHasFocus(bool trueOrFalse){ m_SymbolsWindowHasFocus = trueOrFalse;}
     bool GetSymbolsWindowHasFocus(){return m_SymbolsWindowHasFocus;}
 
+    // Set or return Project that changed "Global setting" in workspace
+    cbProject* GetOptsChangedByProject(){ return m_pOptsChangedProject;}
+    void SetOptsChangedByProject(cbProject* pProject){m_pOptsChangedProject = pProject;}
+    // Set or return Parser that changed "Global setting" in Single File workspace
+    ParserBase* GetOptsChangedByParser(){ return m_pOptsChangedParser;}
+    void SetOptsChangedByParser(ParserBase* pParserBase){m_pOptsChangedParser = pParserBase;}
+    ParserBase* GetTempParser(){return m_TempParser;}
+    ParserBase* GetClosingParser(){return m_pClosingParser;} //(ph 2025/02/12)
+    void        SetClosingParser(ParserBase* pParser){m_pClosingParser = pParser;} //(ph 2025/02/12)
+
 protected:
     /** When a Parser is created, we need a full parsing stage including:
      * 1, parse the priority header files firstly.
@@ -474,11 +484,12 @@
      */
     bool RemoveProjectFromParser(cbProject* project);
 
+
 private:
     typedef std::pair<cbProject*, ParserBase*> ProjectParserPair;
     typedef std::list<ProjectParserPair>       ParserList;
 
-    /** a list holing all the cbp->parser pairs, if in one parser per project mode, there are many
+    /** a list holding all the cbp->parser pairs, if in one parser per project mode, there are many
      * many pairs in this list. In one parser per workspace mode, there is only one pair, and the
      * m_ParserList.begin()->second is the common parser for all the projects in workspace.
      */
@@ -527,6 +538,13 @@
     bool m_ClassBrowserViewIsStale = true;
     bool m_SymbolsWindowHasFocus = false;
 
+    //The latest project to change the .conf file //(ph 2025/02/04)
+    cbProject* m_pOptsChangedProject = nullptr;
+    //The latest parser to change the .conf file //(ph 2025/02/04)
+    ParserBase* m_pOptsChangedParser = nullptr;
+    // the currently closing parser
+    ParserBase* m_pClosingParser = nullptr;
+
 };
 
 #endif // PARSEMANAGER_H
Index: parser/parser.cpp
===================================================================
--- parser/parser.cpp	(revision 13611)
+++ parser/parser.cpp	(working copy)
@@ -32,9 +32,10 @@
 
 #include "parser.h"
 #include "parserthreadedtask.h"
+#include "../parsemanager.h" //(ph 2025/02/04)
 
 #include "../classbrowser.h"
-#include "../classbrowserbuilderthread.h"
+//unused - #include "../classbrowserbuilderthread.h"
 
 
 #ifndef CB_PRECOMP
@@ -921,19 +922,64 @@
 
 void Parser::WriteOptions()
 {
+     // Global settings bug fix (ph 2025/02/12)
+     //https://forums.codeblocks.org/index.php/topic,25955 Hiccups while typing
+
+    // Assemble status to determine if a Parser or Project changed a global setting.
+    ParseManager*   pParseMgr = (ParseManager*)m_Parent;
+    ParserBase*     pTempParser = pParseMgr->GetTempParser();
+    ParserBase*     pClosingParser = pParseMgr->GetClosingParser(); //see ParseManger::DeleteParser()
+    ParserBase*     pCurrentParser = &(pParseMgr->GetParser());     //aka: m_parser
+
+    bool isClosingParser  = pCurrentParser == pClosingParser;
+    bool isTempParser     = pTempParser == pCurrentParser;
+    bool globalOptionChanged = pParseMgr->GetOptsChangedByParser() or pParseMgr->GetOptsChangedByProject();
+
+    // If this is a parser close, do not allow CB global settings writes.
+    bool allowGlobalUpdate = false;
+
+    // When no closing parser, but CB globals were changed, write to .conf
+    if ( (not isClosingParser) and globalOptionChanged)
+        allowGlobalUpdate = true;
+
+    // Closing parsers are not allowed to write to CB globals.
+    //  CB Globals were already written when when user changed the setting.
+    if (isClosingParser) allowGlobalUpdate = false;
+    // If no changes to the CB globals, no need to write
+    if (not globalOptionChanged)
+        allowGlobalUpdate = false; // no global settings have changed
+
     ConfigManager* cfg = Manager::Get()->GetConfigManager(_T("code_completion"));
 
-    // Page "Code Completion"
-    cfg->Write(_T("/use_SmartSense"),                m_Options.useSmartSense);
-    cfg->Write(_T("/while_typing"),                  m_Options.whileTyping);
+    // ----------------------------------------------------------------------------
+    // set any user changed CB global settings for CodeCompletion
+    // ----------------------------------------------------------------------------
+    if (allowGlobalUpdate)
+    {
+        // Page "Code Completion"
+        cfg->Write(_T("/use_SmartSense"),                m_Options.useSmartSense);
+        cfg->Write(_T("/while_typing"),                  m_Options.whileTyping);
 
-    // Page "C / C++ parser"
-    cfg->Write(_T("/parser_follow_local_includes"),  m_Options.followLocalIncludes);
-    cfg->Write(_T("/parser_follow_global_includes"), m_Options.followGlobalIncludes);
-    cfg->Write(_T("/want_preprocessor"),             m_Options.wantPreprocessor);
-    cfg->Write(_T("/parse_complex_macros"),          m_Options.parseComplexMacros);
-    cfg->Write(_T("/platform_check"),                m_Options.platformCheck);
+        // Page "C / C++ parser"
+        cfg->Write(_T("/parser_follow_local_includes"),  m_Options.followLocalIncludes);
+        cfg->Write(_T("/parser_follow_global_includes"), m_Options.followGlobalIncludes);
+        cfg->Write(_T("/want_preprocessor"),             m_Options.wantPreprocessor);
+        cfg->Write(_T("/parse_complex_macros"),          m_Options.parseComplexMacros);
+        cfg->Write(_T("/platform_check"),                m_Options.platformCheck);
 
+    }
+    // clean out any parser flags used to guard CB global settings
+    pParseMgr->SetOptsChangedByParser(nullptr);  //(ph 2025/02/12)
+    pParseMgr->SetOptsChangedByProject(nullptr); //(ph 2025/02/12)
+    pParseMgr->SetClosingParser(nullptr);
+    // if currrent parser == TempParser, force it to update, else the next
+    // display of the setting dialog will show TempParser stale cached settings.
+    if (isTempParser) pTempParser->ReadOptions();
+
+    // ----------------------------------------------------------------------------
+    // set any user changed ClassBrowser settings
+    // ----------------------------------------------------------------------------
+
     // Page "Symbol browser"
     cfg->Write(_T("/browser_show_inheritance"),      m_BrowserOptions.showInheritance);
     cfg->Write(_T("/browser_expand_ns"),             m_BrowserOptions.expandNS);
Index: resources/manifest.xml
===================================================================
--- resources/manifest.xml	(revision 13611)
+++ resources/manifest.xml	(working copy)
@@ -3,7 +3,7 @@
     <SdkVersion major="1" minor="10" release="0" />
     <Plugin name="CodeCompletion">
         <Value title="Code completion" />
-        <Value version="1.0.5 24/01/29" />
+        <Value version="1.0.6 25/02/12" />
         <Value description="This plugin provides a symbols browser for your projects and code-completion inside the editor.
 
 

[ollydbg]

Hi, thanks for your work.

I tested this patch. One issue I see is:

when I have projectA loaded, and later projectB loaded. Now, if I active projectB active, and I open the CC setting(option) dialog, change some settings, and click the apply button, the settings were saved to the .conf file. In this time, the ParserA and ParserB actually have different Parser settings. Now, if I switch the active Parser to projectA, and if I open the CC option dialog again, the setting is the new modified projectB's CC settings. Suppose in this case, the ParserA already finished its parsing, and its Parsing option is not the options when you see in the CC setting dialog.

So, what is the time we need to "read the Parser option from the .conf to the Parser instance", I think it should be in the constructor, and this should be the only place a Parser instance to read the initial parsing option.

Another thing I see which may be not correct (in the old code, not in your patch) is in this function:

Code

// ----------------------------------------------------------------------------
void ClassBrowser::SetParser(ParserBase* parser)
// ----------------------------------------------------------------------------
{
    if (m_Parser == parser)
        return;

    m_Parser = parser;
    if (m_Parser)
    {
        const int sel = XRCCTRL(*this, "cmbView", wxChoice)->GetSelection();
        BrowserDisplayFilter filter = static_cast<BrowserDisplayFilter>(sel);
        if (!m_ParseManager->IsParserPerWorkspace() && filter == bdfWorkspace)
            filter = bdfProject;

        m_Parser->ClassBrowserOptions().displayFilter = filter;
        m_Parser->WriteOptions();
        UpdateClassBrowserView();
    }
    else
        CCLogger::Get()->DebugLog("SetParser: No parser available.");
}

When we switch the Parser in the class browser, why do we need to "WriteOptions()"? I think we should only call something like "WriteClassBrowserOptions()", because I think switch the Parser only need to write the ClassBrowserOptions to the .conf file, but not the whole Parser options.

[Pecan]

So, what is the time we need to "read the Parser option from the .conf to the Parser instance", I think it should be in the constructor, and this should be the only place a Parser instance to read the initial parsing option.

I don't understand this request at all.
 
If a parser only reads the settings at parser creation time, when the user later changes the global settings but the parser is not allowed to read them after the change, the user now is parsing without the settings he/she requested.

If that user was me, I'd call that a bug.

I think it would be better to throw up a message box saying "Sorry, sniff, global settings will not be applied for a project/parser until you request a re-parse of all the other loaded projects." Just kidding...

This is a hard problem. If we don't apply changes, we violate UI standards. If we do change them, a busy parser won't apply them anyway.

Maybe we should put up a message box that says, these changes will only be applied on the next re-parse for a project.
We could list the projects that have a 'busy' not done flag.

I remember this situation from about 13 years ago. The client asked me to throw up a message box that said "Options cannot be changed while a workspace is loaded".

[ollydbg]

If a parser only reads the settings at parser creation time, when the user later changes the global settings but the parser is not allowed to read them after the change, the user now is parsing without the settings he/she requested.

If that user was me, I'd call that a bug.

My initial thought is that:

When user loads a cbp, the Parser created, and the global CC option is copied to the Parser's member variable(m_Options or other names). A TempParser is created when CC plugin get initialized, this Parser serves for the standalone cpp files parsing.

When project A's CC option dialog get opened, it shows the Parser A's member variable m_Options, and when user click the Apply button, its m_Options are updated, and the global options inside .conf file also get updated.

When we switch to project B, when I open the CC option dialog, it would show:

option1: Parser B's m_Options, because m_Options in Parser B is originally a copy of .conf when it gets created, so it is before we modify the "project A's CC options".

option2: Parser B's m_Options should reread from the .conf file, so that the global CC option was copied to project B.

From your point of view, you think the "option2" is the correct option. I just think a while, and I think you are correct, that is "All the Parser instances now share a same CC options".

I think it would be better to throw up a message box saying "Sorry, sniff, global settings will not be applied for a project/parser until you request a re-parse of all the other loaded projects." Just kidding...

This is a hard problem. If we don't apply changes, we violate UI standards. If we do change them, a busy parser won't apply them anyway.

Maybe we should put up a message box that says, these changes will only be applied on the next re-parse for a project.
We could list the projects that have a 'busy' not done flag.

I remember this situation from about 13 years ago. The client asked me to throw up a message box that said "Options cannot be changed while a workspace is loaded".

Yes, I agree. Changing some of the parsing option does not take effect immediately.

Even I have change the project A's CC option, and the parser A is already finished its parsing job, so to make the parser option get affect, we have to "reparse" the project.

As a convolution, I think you patch is OK! Thanks for so many times to tweak your patches.

The remaining issue is: since all the Parsers were sharing a single CC option, why do we keep so many copies? Maybe, we can just have one in the ParseManager class. 

[Pecan]

@ ollydbg

Thanks for all your help and excellent guidance.
I decided to break the changes up into two tasks.

I've committed what we have. (svn 13612) and will continue with your further suggestions.
But after I study CC some more and get more confident about your suggestions.



Thanks,

[ollydbg]

OK, thanks, I have fixed an issue mentioned here: Re: The 14 June 2024 build (13529) is out.

BTW: why do you have so many custom comments in the source code, do you added manually or by some tools?

Code

// ----------------------------------------------------------------------------
void ClassBrowser::SetParser(ParserBase* parser)
// ----------------------------------------------------------------------------

[Pecan]

OK, thanks, I have fixed an issue mentioned here: Re: The 14 June 2024 build (13529) is out.

BTW: why do you have so many custom comments in the source code, do you added manually or by some tools?

Code

// ----------------------------------------------------------------------------
void ClassBrowser::SetParser(ParserBase* parser)
// ----------------------------------------------------------------------------

They're applied by a key macro I have on my local system.
Once I can see where I'm supposed to be working, I mark it so it doesn't take so long to find it again.

Sometimes, I forget to remove the taggings.

I do it because of my eye sight. I have a terrible time seeing/differentiating code where I'm working without marking where I'm coding or debugging.

If it bothers someone, they can remove the comments or debugging tags. I'll try to do better at remembering it myself before a commit.

I forgot to do it this time because I'm working on 'stage two' of the suggested changes.

[Elena]

So, since the latest NB finally came out I could test the fixes.
It *seems* there are no more hiccups, but I shall test it some longer. I will update this thread in case I still find some issue, in the meanwhile thanks for fixing !