-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Setting dimension with unit #1434
Conversation
@@ -18,7 +18,7 @@ public class UnitsEditorPresenter : AbstractDisposablePresenter<IUnitsEditorView | |||
|
|||
private bool _columnMapping; | |||
|
|||
public UnitDescription Unit => new UnitDescription(_selectedUnit, _selectedColumn); | |||
public UnitDescription Unit => new UnitDescription(_view.SelectedUnit, _selectedColumn); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use _selectedUnit. Why are they out of sync? This smells. Remove the _selectedUnit if it's not used
@@ -36,6 +36,11 @@ public UnitsEditorView() | |||
_columnComboBox.EditValueChanged += (s, a) => OnEvent(onColumnComboBoxTextChanged); | |||
} | |||
|
|||
public string SelectedUnit | |||
{ | |||
get => _unitComboBox.EditValue as string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why isn't it bound to an object like we always do?
column.Unit = _mappingParameterEditorPresenter.Unit; | ||
column.Dimension = _mappingParameterEditorPresenter.Dimension; | ||
column.Unit = _mappingParameterEditorPresenter.Unit; | ||
column.Dimension = _columnInfos |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
THis set needs to be commented. too much nested info that makes it very hard to follow.
Specifically, why is _mappingParameterEditorPresenter.Dimension;
not enough
also you are using, FIrst, are you sure to find one?.
Last when comparing , we use string.equals which deals with null properly (I may be wrong but I think == does not) . To verify :)
@@ -212,6 +215,11 @@ public void UpdateDescriptionForModel(MappingDataFormatParameter mappingSource) | |||
_view.CloseEditor(); | |||
} | |||
|
|||
private bool dimensionContainsUnit(IDimension dimension, UnitDescription unit) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have a HasUnit.. why do you need to rewrite it >
@@ -36,6 +36,12 @@ public UnitsEditorView() | |||
_columnComboBox.EditValueChanged += (s, a) => OnEvent(onColumnComboBoxTextChanged); | |||
} | |||
|
|||
public string SelectedUnit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not know why we are using something like this instead of usual binding to an object. You are breaking the pattern we are using everywhere. I am going to merge because this PR has been open for too long It would be good to try to address the comments in PR (either to explain why this is done like this or implement the requested/proposed changes) a bit quicker if possible
Fixes #1405