fix: apply the colour picked in the property grid Color/TabColor fields - #156
Open
jafin wants to merge 2 commits into
Open
fix: apply the colour picked in the property grid Color/TabColor fields#156jafin wants to merge 2 commits into
jafin wants to merge 2 commits into
Conversation
Picking a colour for a connection's Color or TabColor failed with "Property value is not valid" / "Object of type 'System.Drawing.Color' cannot be converted to type 'System.String'", and the colour was not set. Both properties are strings but were annotated with the stock ColorEditor, which returns a System.Drawing.Color. The PropertyGrid commits an editor's return value straight through PropertyDescriptor.SetValue and never runs the TypeConverter on that path, so the assignment threw. TabColorConverter only covered the text-entry path, which is why the error survived it. - Add ColorStringEditor, wrapping ColorEditor for string-backed colour properties: it feeds the editor a Color and translates the picked colour back to the stored string form. It also restores the swatch preview, which was blank because ColorEditor cannot paint a string. - Point Color and TabColor at the new editor. - Return the standard-values dropdown entries as strings; picking one from the list failed in exactly the same way. - Convert Color.Empty to an empty string instead of "#00000000" so clearing a colour leaves the property unset.
PR Summary by QodoFix PropertyGrid color picker for string-backed Color/TabColor properties
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
There was a problem hiding this comment.
Pull request overview
Fixes PropertyGrid color editing for string-backed connection color fields in mRemoteNG by ensuring the UI editor and standard-value dropdowns commit string values (not System.Drawing.Color), preventing assignment exceptions and allowing the selected color to be applied correctly.
Changes:
- Added
ColorStringEditorto bridge betweenColorEditor(Color) and string-backed properties (string). - Updated
AbstractConnectionRecord.Colorand.TabColorto use the new editor. - Adjusted
TabColorConverterto return string standard values and to mapColor.Emptyto""; added NUnit coverage.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| mRemoteNG/UI/Controls/ConnectionInfoPropertyGrid/ColorStringEditor.cs | New UITypeEditor wrapper converting between string storage and ColorEditor behavior. |
| mRemoteNG/Connection/AbstractConnectionRecord.cs | Swapped PropertyGrid editor for Color / TabColor to the new string-aware editor. |
| mRemoteNG/Tools/MiscTools.cs | Updated TabColorConverter empty-color handling and standard values to be strings. |
| mRemoteNGTests/UI/Controls/ColorStringEditorTests.cs | Added tests covering editor usage and dropdown standard value typing. |
| mRemoteNGTests/Tools/TabColorConverterTests.cs | Added regression test for Color.Empty → "". |
Comment on lines
+20
to
+24
| public override object? EditValue(ITypeDescriptorContext? context, IServiceProvider provider, object? value) | ||
| { | ||
| object? editedValue = base.EditValue(context, provider, ToColor(value)); | ||
| return editedValue is Color color ? Converter.ConvertFrom(color) : value; | ||
| } |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+20
to
+23
| public override object? EditValue(ITypeDescriptorContext? context, IServiceProvider provider, object? value) | ||
| { | ||
| object? editedValue = base.EditValue(context, provider, ToColor(value)); | ||
| return editedValue is Color color ? Converter.ConvertFrom(color) : value; |
There was a problem hiding this comment.
1. Colorstringeditor overrides missing xmldocs 📘 Rule violation ⚙ Maintainability
ColorStringEditor adds public override members (EditValue, PaintValue) without required XML documentation comments. This reduces API documentation quality and violates the project’s public-API documentation requirement.
Agent Prompt
## Issue description
`ColorStringEditor` introduces public override members without XML documentation comments (`/// <summary>...</summary>`), which violates the requirement to document all public API members.
## Issue Context
The class has a `<summary>`, but the public methods `EditValue` and `PaintValue` do not.
## Fix Focus Areas
- mRemoteNG/UI/Controls/ConnectionInfoPropertyGrid/ColorStringEditor.cs[20-29]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+37
to
+40
| public void EditValueReturnsAString(string value) | ||
| { | ||
| var result = _editor.EditValue(null, new EmptyServiceProvider(), value); | ||
| Assert.That(result, Is.EqualTo(value)); |
There was a problem hiding this comment.
2. Colorstringeditortests names lack pattern 📘 Rule violation ▣ Testability
New NUnit tests use method names that do not follow the required MethodName_Scenario_ExpectedBehavior underscore pattern, which makes test intent less consistent and violates the test naming convention rule. This includes methods in ColorStringEditorTests as well as the newly added ConvertFromEmptyColorReturnsEmptyString test.
Agent Prompt
## Issue description
Several newly-added NUnit test method names do not follow the required `MethodName_Scenario_ExpectedBehavior` naming convention (three segments separated by exactly two underscores).
## Issue Context
The naming rule expects three underscore-delimited segments to make test intent consistent. Current examples that violate the rule include methods like `EditValueReturnsAString`, `EditValueOfNullDoesNotThrow`, and the added `ConvertFromEmptyColorReturnsEmptyString`, all of which have no underscores and therefore do not satisfy the convention.
## Fix Focus Areas
- mRemoteNGTests/UI/Controls/ColorStringEditorTests.cs[37-64]
- mRemoteNGTests/Tools/TabColorConverterTests.cs[58-63]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+11
to
+14
| namespace mRemoteNGTests.UI.Controls | ||
| { | ||
| [NUnit.Framework.Apartment(System.Threading.ApartmentState.STA)] | ||
| public class ColorStringEditorTests |
There was a problem hiding this comment.
3. Colorstringeditortests path not mirrored 📘 Rule violation ▣ Testability
ColorStringEditor production code is added under mRemoteNG/UI/Controls/ConnectionInfoPropertyGrid/, but its test is placed under mRemoteNGTests/UI/Controls/ without the matching subdirectory. This violates the requirement to mirror production directory structure in the test project.
Agent Prompt
## Issue description
The new test file location does not mirror the production folder hierarchy for `ColorStringEditor`.
## Issue Context
Production file path: `mRemoteNG/UI/Controls/ConnectionInfoPropertyGrid/ColorStringEditor.cs`.
Test file path currently: `mRemoteNGTests/UI/Controls/ColorStringEditorTests.cs`.
## Fix Focus Areas
- mRemoteNGTests/UI/Controls/ColorStringEditorTests.cs[11-15]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
TabColorConverter maps text it cannot parse to Color.Empty rather than throwing, so a legacy or hand-edited entry such as "not-a-color" reached the wrapped ColorEditor as an empty colour. Dismissing the picker echoed that empty colour straight back, and it was then stored as an empty string, silently discarding the original value. Track whether the stored value actually describes a colour, and keep the value untouched when it does not and no colour was picked. A genuinely unset value - null or blank - still round-trips to an empty string.
Author
|
Follow-up pushed:
Covered by new cases in |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Setting Color or Tab Color on a connection via the property-grid picker fails with
Property value is not valid/Object of type 'System.Drawing.Color' cannot be converted to type 'System.String', and the colour is never applied.Root cause
Both properties are
string, but they were annotated with the stockSystem.Drawing.Design.ColorEditor, which returns aSystem.Drawing.Color. The PropertyGrid commits an editor's return value straight throughPropertyDescriptor.SetValueand never runs theTypeConverteron that path, so the assignment throws.MiscTools.TabColorConverteralready handledColor→string, but only the text-entry path goes through it — which is why the error survived that earlier fix.Changes
ColorStringEditor(new) — wrapsColorEditorfor string-backed colour properties: feeds the editor aColor, translates the picked colour back into the stored string form. Also restores the swatch preview, which was blank becauseColorEditorcannot paint a string value.AbstractConnectionRecord.Color/.TabColornow use the new editor.TabColorConverter.GetStandardValuesreturns strings instead ofColorobjects — picking an entry from the dropdown list failed in exactly the same way.TabColorConverter.ConvertFrom(Color.Empty)now yields""instead of"#00000000", so clearing a colour leaves the property unset.Verification
Full MSBuild build succeeds.
Note for reviewers: the NUnit suite could not be executed on my machine — every group reports
NUnit couldn't run the N discovered tests: Only supported on Windows10.0.26100.0, becausemRemoteNGTests.csprojsetsSupportedOSPlatformVersion 10.0.26100.0while the host is Windows 10.0.22631. That block is pre-existing and assembly-wide (it hits untouched groups such as Config and Security identically) and unrelated to this change, so I left the test project's target platform alone. CI should run the suite normally.To verify behaviour regardless, I ran the equivalent assertions against the built
mRemoteNG.dllfrom a standalone harness — all pass:"Red","#804020",""andnullColorStringEditorSetValuewith the editor's output stores"Purple"instead of throwingNew tests added:
mRemoteNGTests/UI/Controls/ColorStringEditorTests.cs(non-interactive — noIWindowsFormsEditorServiceis supplied, so no dialog is shown) plus an empty-colour case inTabColorConverterTests.