Implement User Selectable Code Editor Style in cppcheck-gui by scottfurry · Pull Request #1913 · cppcheck-opensource/cppcheck

scottfurry · 2019-06-21T20:24:06Z

Building on #1874, commit adds user controls to choose
or edit style in cppcheck-gui ONLY. Commit does not
address CodeEditor style usage in triage app at this time.

Code Editor style can be altered from the added "Code Editor"
tab in the user preferences. The user has the option to select
default light, default dark, or to customize.

If user leaves the style set to light or dark defaults, this
will be reflected in the choices shown in the preferences
dialog.

User choice for Code Editor Style is saved in the cppcheck-gui
preferences under the heading "EditorStyle".

scottfurry · 2019-06-22T14:55:43Z

CI testing failed - logs indicated left over testing/debug code. Removed.

scottfurry · 2019-06-22T16:10:26Z

CI highlighted warnings about variable naming conventions. Updated to correct code to expected style.

danmar

I am eager to get this in. However I have some small nits..

danmar · 2019-06-23T08:39:41Z

+CodeEditor::~CodeEditor()
+{
+    // NOTE: not a Qt Object - delete manually
+    if( mWidgetStyle ) delete mWidgetStyle;


Skip the redundant check.

danmar · 2019-06-23T08:40:01Z

-    else mWidgetStyle = new CodeEditorStyle(defaultStyle);
+    switch( rule.ruleRole )
+    {
+        case( RuleRole::Keyword ):


imho the parentheses here does not help

danmar · 2019-06-23T08:42:32Z

+    return !(*this == rhs);
+}
+
+CodeEditorStyle CodeEditorStyle::LoadSettings( QSettings *pSettings )


Please use camelCase for function names.

I would prefer the name settings instead of pSettings.

danmar · 2019-06-23T08:44:07Z

+CodeEditorStyle CodeEditorStyle::LoadSettings( QSettings *pSettings )
+{
+    CodeEditorStyle theStyle( defaultStyleLight );
+    if( pSettings) {


I suggest a early return here.

danmar · 2019-06-23T08:48:09Z

+                                    const CodeEditorStyle& theStyle )
+{
+    if( pSettings) {
+        bool bDefaultLight = ( defaultStyleLight == theStyle );


I prefer that the name bDefaultLight is changed to isDefaultLight

scottfurry · 2019-06-23T12:25:14Z

All changes identified above incorporated.

scottfurry · 2019-06-23T13:39:33Z

One more...
Change not identified but was similar in nature to change identified above.

danmar

some more nits.

danmar · 2019-06-23T13:33:58Z

@@ -0,0 +1 @@
+/home/scottfurry/FurCaT_Remote/CPPCheck/project/.vscode


remove this file from the PR

You're right. That shouldn't be there!

danmar · 2019-06-23T13:34:57Z

+        case RuleRole::Symbol:
+            rule.format = mSymbolFormat;
+            break;
+        default:


hmm.. I have the feeling that it's better to remove the "default". then we'll get compiler warnings if an enum value is missing.

Would this not raise another warning about a missing default in switch statement?

Yes.. but I rather suppress that warning and get warnings when I forget enum constants instead.

danmar · 2019-06-23T13:36:17Z

+    CodeEditorStyle theStyle( defaultStyleLight );
+    if( !settings) return theStyle;
+
+    if( settings->childGroups().contains( SETTINGS_STYLE_GROUP ))


It seems you can have an early return here also.

danmar · 2019-06-23T13:41:55Z

+
+bool CodeEditorStyle::operator==( const CodeEditorStyle& rhs ) const
+{
+    if( this->widgetFGColor != rhs.widgetFGColor ) return false;


could you remove "this->" in these conditions.

danmar · 2019-06-23T13:42:21Z

+}
+
+void SelectColorButton::changeColor() {
+    QColorDialog *pDlg = new QColorDialog( mColor );


As far as I know you don't need to use new/delete for QColorDialog in this method.

QColorDialog dlg(mColor);

danmar · 2019-06-23T13:42:36Z

+}
+
+void SelectColorButton::setColor( const QColor& color ) {
+    this->mColor = color;


Remove "this" here.

danmar · 2019-06-23T13:44:25Z

+    mSymbolFormat.setForeground( newStyle.symbolFGColor );
+    mSymbolFormat.setBackground( newStyle.symbolBGColor );
+    mSymbolFormat.setFontWeight( newStyle.symbolWeight );
+    for( QVector<HighlightingRule>::iterator ruleIT = mHighlightingRules.begin();


I wonder if you can't use foreach here.

Tried that. Qt foreach macro wants to use a const variable in each iteration. That becomes a problem since contents are being altered in the call to apply the style. There is the std::foreach or std::for(auto &: <container) but IIRC that is only available >=c++11.

ok.. please try C++11 range for if you can. We use that throughout the code in cppcheck-core so you can't build the GUI unless the compiler support that anyway.

That's going to change a couple of things in the .pro and CMakeList.txt files. Are you okay with that?

ok.. let's keep it like this for now.. and then somebody can try to modernize for loops later in a separate PR.

Already had change incorporated and doing a full build for testing. Nothing jumps out in the logging messages. Expect this to be incorporated in final push. Only change was in .pro file where one line for gcc was using c++0x.

danmar · 2019-06-23T15:23:45Z

I really look forward to the day when the full GUI uses a configurable theme! :-)

scottfurry · 2019-06-23T15:51:12Z

All identified changes in code made.

Building on #1874, commit adds user controls to choose or edit style in cppcheck-gui ONLY. Commit does not address CodeEditor style usage in triage app at this time. Code Editor style can be altered from the added "Code Editor" tab in the user preferences. The user has the option to select default light, default dark, or to customize. If user leaves the style set to light or dark defaults, this will be reflected in the choices shown in the preferences dialog. User choice for Code Editor Style is saved in the cppcheck-gui preferences under the heading "EditorStyle".

danmar

If Travis/Appveyor finish happily I think we can merge this.

scottfurry · 2019-06-23T16:59:19Z

\o/ All checks passed!

Styling change missed in #1913.

…-opensource#1913) Building on cppcheck-opensource#1874, commit adds user controls to choose or edit style in cppcheck-gui ONLY. Commit does not address CodeEditor style usage in triage app at this time. Code Editor style can be altered from the added "Code Editor" tab in the user preferences. The user has the option to select default light, default dark, or to customize. If user leaves the style set to light or dark defaults, this will be reflected in the choices shown in the preferences dialog. User choice for Code Editor Style is saved in the cppcheck-gui preferences under the heading "EditorStyle".

…k-opensource#1923) Styling change missed in cppcheck-opensource#1913.

danmar reviewed Jun 23, 2019

View reviewed changes

danmar approved these changes Jun 23, 2019

View reviewed changes

danmar merged commit 0d76d07 into cppcheck-opensource:master Jun 23, 2019

scottfurry deleted the EditStyle branch June 23, 2019 17:13

scottfurry mentioned this pull request Jun 25, 2019

Propagate Monospaced Font to Line Number Area in Code Editor #1923

Merged

danmar pushed a commit that referenced this pull request Jun 25, 2019

Propagate Monospaced Font to Line Number Area in Code Editor (#1923)

158194d

Styling change missed in #1913.

jubnzv pushed a commit to jubnzv/cppcheck that referenced this pull request Nov 13, 2019

Propagate Monospaced Font to Line Number Area in Code Editor (cppchec…

64e4e89

…k-opensource#1923) Styling change missed in cppcheck-opensource#1913.

		@@ -0,0 +1 @@
		/home/scottfurry/FurCaT_Remote/CPPCheck/project/.vscode No newline at end of file

Conversation

scottfurry commented Jun 21, 2019

Uh oh!

scottfurry commented Jun 22, 2019

Uh oh!

scottfurry commented Jun 22, 2019

Uh oh!

danmar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scottfurry commented Jun 23, 2019

Uh oh!

scottfurry commented Jun 23, 2019

Uh oh!

danmar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danmar commented Jun 23, 2019

Uh oh!

scottfurry commented Jun 23, 2019

Uh oh!

danmar left a comment

Choose a reason for hiding this comment

Uh oh!

scottfurry commented Jun 23, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants