Code::Blocks Forums

Developer forums (C::B DEVELOPMENT STRICTLY!) => Development => Topic started by: ollydbg on October 14, 2013, 09:47:12 am

Title: Question about rev 9399
Post by: ollydbg on October 14, 2013, 09:47:12 am
Hi, alpha, I see rev 9399, I have some comments on this:
In rev 8332, I introduce the "auto" mode for automatically detect the eol mode by scanning the files.
Which I have:
Code
@@ -1575,6 +1665,14 @@ void cbEditor::InternalSetEditorStyleAfterFileOpen(cbStyledTextCtrl* control)
 
     ConfigManager* mgr = Manager::Get()->GetConfigManager(_T("editor"));
 
+    // set the EOL, fall back value: Windows takes CR+LF, other platforms LF only
+    int eolMode = mgr->ReadInt(_T("/eol/eolmode"), platform::windows ? wxSCI_EOL_CRLF : wxSCI_EOL_LF);
+
+    if (eolMode == 3) //auto detect the EOL
+        eolMode = DetectLineEnds(control);
+
+    control->SetEOLMode(eolMode);
+
     // Interpret #if/#else/#endif to grey out code that is not active
     control->SetProperty(_T("lexer.cpp.track.preprocessor"), mgr->ReadBool(_T("/track_preprocessor"), true) ? _T("1") : _T("0"));

When an editor is opened, I detect the eol mode, and set this mode by Control->SetEOLMode().

Now, what you change in the code looks like "use the system default eol if the setting is auto", see:
Code
 src/plugins/contrib/wxSmith/wxscoder.cpp | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/plugins/contrib/wxSmith/wxscoder.cpp b/src/plugins/contrib/wxSmith/wxscoder.cpp
index 313929e..8a68f18 100644
--- a/src/plugins/contrib/wxSmith/wxscoder.cpp
+++ b/src/plugins/contrib/wxSmith/wxscoder.cpp
@@ -517,7 +517,10 @@ wxString wxsCoder::RebuildCode(wxString& BaseIndentation,const wxChar* Code,int
 
     if ( EOL.IsEmpty() )
     {
-        int EolMode = Manager::Get()->GetConfigManager(_T("editor"))->ReadInt(_T("/eol/eolmode"), 0);
+        static const int defEol = platform::windows ? wxSCI_EOL_CRLF : wxSCI_EOL_LF;
+        int EolMode = Manager::Get()->GetConfigManager(_T("editor"))->ReadInt(_T("/eol/eolmode"), defEol);
+        if (EolMode == 3) // auto-detect EOL
+            EolMode = defEol;
         switch ( EolMode )
         {
             case 1:  EOL = _T("\r"); break;

So the suggest way is:
If the option is "auto" in the settings, and the file is already opened in the editor, use the eol of the control directly, because the DetectLineEnds(control) function is already run, the eol is correctly set for the current control.

If the file is not opened, and I think you should use the similar way of DetectLineEnds(control), then set the correct eol mode for the generated code.

If the file is empty (like the one going to create a new file), thus we can use system default eol for "auto" mode.

The reason is that I can have many LF source files under Windows system, in this case, I would like wxsmith to generate code in LF mode, this avoid a mixed eol issue.
Title: Re: Question about rev 9399
Post by: Alpha on October 14, 2013, 02:28:06 pm
The two functions that call wxsCoder::RebuildCode() (ApplyChangesEditor() and ApplyChangesString()) both run detection on their given context, and pass the detected EOL to RebuildCode().  The system default setting is only used if the previous detection has failed.
Class and scripted wizards do not have any obvious context to try to guess the desired EOL style from, so I set them to handle "auto" with system default.

Is there a better way to handle these cases?
Title: Re: Question about rev 9399
Post by: ollydbg on October 14, 2013, 03:08:55 pm
Quote from: Alpha on October 14, 2013, 02:28:06 pm
The two functions that call wxsCoder::RebuildCode() (ApplyChangesEditor() and ApplyChangesString()) both run detection on their given context, and pass the detected EOL to RebuildCode().  The system default setting is only used if the previous detection has failed.
Class and scripted wizards do not have any obvious context to try to guess the desired EOL style from, so I set them to handle "auto" with system default.
Ok, thanks for the explanation.

Quote
Is there a better way to handle these cases?
I can't find a better way than what you did. :D
Title: Re: Question about rev 9399
Post by: oBFusCATed on October 14, 2013, 09:50:15 pm
Alpha: Could you wrap this code in a function? It seems the code is similar at the places you've changed.
Title: Re: Question about rev 9399
Post by: Alpha on October 15, 2013, 04:35:57 am
Quote from: oBFusCATed on October 14, 2013, 09:50:15 pm
Alpha: Could you wrap this code in a function?
Okay.  I will add it as a global (unless you think it should belong to cbEditor or something else instead?).
Title: Re: Question about rev 9399
Post by: oBFusCATed on October 15, 2013, 10:13:17 am
I'm not sure where is a good place to put this function. So do what you think it is best.
Title: Re: Question about rev 9399
Post by: Alpha on October 18, 2013, 04:13:40 am
Attached patch adds this function.  I am not thinking very clearly right now, so I will leave this here for a bit before committing it.
Title: Re: Question about rev 9399
Post by: ollydbg on October 18, 2013, 04:26:40 am
Quote from: Alpha on October 18, 2013, 04:13:40 am
Attached patch adds this function.  I am not thinking very clearly right now, so I will leave this here for a bit before committing it.
Good, Maybe "GetEolStr" should be "GetEOLStr"?

EDIT: We have already functions like "SetEOLMode".
Title: Re: Question about rev 9399
Post by: Alpha on October 20, 2013, 01:22:41 am
Renamed and committed.
Title: Re: Question about rev 9399
Post by: oBFusCATed on October 21, 2013, 05:00:35 pm
Please use enum instead of int for the parameter of "wxString GetEOLStr(int eolMode)".
Title: Re: Question about rev 9399
Post by: Alpha on October 21, 2013, 07:02:57 pm
I used int because it was defined as:
Code: sdk/wxscintilla/include/wx/wxscintilla.h:79
#define wxSCI_EOL_CRLF 0
#define wxSCI_EOL_CR 1
#define wxSCI_EOL_LF 2
Should I create an enum with these values?
Title: Re: Question about rev 9399
Post by: oBFusCATed on October 21, 2013, 07:34:33 pm
Then probably it is better to add documentation for the expected values...