Implemented ComboBox #1070
Implemented ComboBox #1070
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1070 +/- ##
==========================================
- Coverage 43.41% 43.33% -0.08%
==========================================
Files 542 543 +1
Lines 20718 20780 +62
Branches 3018 3029 +11
==========================================
+ Hits 8994 9006 +12
- Misses 10877 10927 +50
Partials 847 847
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #1070 +/- ##
=========================================
Coverage ? 44.84%
=========================================
Files ? 539
Lines ? 21178
Branches ? 3183
=========================================
Hits ? 9498
Misses ? 10778
Partials ? 902
Continue to review full report at Codecov.
|
|
Thanks @jp2masa! I've been on vacation and just catching up now. I will review this asap! |
There was a problem hiding this comment.
I added a ComboBoxPage to the ControlCatalog with a simple ComboBox:
<UserControl xmlns="https://github.com/avaloniaui">
<StackPanel Orientation="Vertical" Gap="4">
<TextBlock Classes="h1">ComboBox</TextBlock>
<TextBlock Classes="h2">A combination of a drop down list and a text box control</TextBlock>
<StackPanel Orientation="Vertical"
Margin="0,16,0,0"
HorizontalAlignment="Center"
Gap="16">
<ComboBox>
<ComboBoxItem>Item 1</ComboBoxItem>
<ComboBoxItem>Item 2</ComboBoxItem>
<ComboBoxItem>Item 3</ComboBoxItem>
</ComboBox>
</StackPanel>
</StackPanel>
</UserControl>
And when I run I get the following exception:
System.ArgumentException occurred
HResult=0x80070057
Message=Value does not fall within the expected range.
Source=Avalonia.Base
StackTrace:
at Avalonia.Contract.Requires[TException](Boolean condition) in D:\projects\Avalonia\src\Avalonia.Base\Contract.cs:line 33
Could you add a page for the control to the ControlCatalog (or I can push the page I added to your branch if you like) and fix the exception?
There was a problem hiding this comment.
Thanks for fixing that!
Found another problem - this test fails:
[Fact]
public void Text_Is_Selected_String_Item()
{
var items = new[] { "A", "B", "C", "D" };
var target = new ComboBox
{
Items = items,
SelectedIndex = 2
};
var text = target.GetValue(ComboBox.TextProperty);
Assert.True(text == "C");
}
You'll see that items can be of any type, they don't have to be ComboBoxItems.
Secondly, could you merge the latest master into your branch? I would do it myself but you don't seem to have granted me write access to your fork branch.
|
I think that ComboBox is only supposed to deal with text, specified in |
|
No, that's not quite how it works :) For example you can bind the This is a very common scenario (more common than using |
There was a problem hiding this comment.
A few more things, sorry :(
If I create a ComboBox like this:
<ComboBox MinWidth="128" Text="Custom Text">
<ComboBoxItem>
<Panel>
<Rectangle Fill="{StyleResource ThemeAccentBrush}"/>
<TextBlock Margin="8">Control Items</TextBlock>
</Panel>
</ComboBoxItem>
<ComboBoxItem>Item 2</ComboBoxItem>
<ComboBoxItem>Item 3</ComboBoxItem>
</ComboBox>
And select the first item, I get a System.NotSupportedException exception thrown, however this works in WPF - the combo box text is set to System.Windows.Controls.ComboBoxItem.
Also in WPF if I type the text of an item into the ComboBox, that item becomes selected, which doesn't happen here.
src/Avalonia.Controls/ComboBox.cs
Outdated
| Contract.Requires<NotSupportedException>(!(item is IControl)); | ||
| } | ||
|
|
||
| Text = Convert.ToString(item); |
There was a problem hiding this comment.
I think you can just use item?.ToString() here.
|
|
||
| foreach (var item in Items) | ||
| { | ||
| if (item.ToString() == _textBox.Text) |
There was a problem hiding this comment.
ToString() may not represent the item correctly. You need to check MemberSelector. Also, item may be null.
|
|
||
| foreach (var item in Items) | ||
| { | ||
| if (item.ToString() == _textBox.Text) |
There was a problem hiding this comment.
String comparison should be case insensitive
|
|
||
| foreach (var item in Items) | ||
| { | ||
| if (item.ToString() == _textBox.Text) |
There was a problem hiding this comment.
If there's no exact match, the first item that starts with the text should be selected. For example, if the list contains "Item1", "Item2", "Item33", , "Item34", when typing "I", "Item1" would be selected, and when typing "Item3", "Item 33" would be selected. If possible, please unit test those.
| /// <inheritdoc/> | ||
| protected override void OnPointerPressed(PointerPressedEventArgs e) | ||
| { | ||
| if (!IsDropDownOpen && ((IVisual)e.Source).GetVisualRoot() != typeof(PopupRoot)) |
There was a problem hiding this comment.
The drop down should be opened by the toggle button, not when clicking anywhere else, so I think this check is redundant.
|
|
||
| private void UpdateSelectionBoxText(object item) | ||
| { | ||
| Text = item?.ToString(); |
There was a problem hiding this comment.
Again, use MemberSelector instead of ToString()
| @@ -0,0 +1,26 @@ | |||
| <Styles xmlns="https://github.com/avaloniaui"> | |||
| <Style Selector="ComboBoxItem"> | |||
| <Setter Property="Padding" Value="2"/> | |||
There was a problem hiding this comment.
Add <Setter Property="Background" Value="Transparent"/>
|
@jp2masa thanks for this PR, but judging from the fact that there have been multiple problems, I'm going to make a guess and say that you didn't implement it because you needed the functionality? It's just I'm wondering if an old-style |
|
I don't need the functionality. I was just trying to help, so I was looking for up for grabs issues and I decided to try this one. |
|
Hmm yeah, we probably shouldn't have marked that issue as
Do you want to keep going ahead with this PR? It's probably going to be a long slog unless you have an application that actually needs the functionality as the back-and-forth here has shown. In addition, as I mentioned, personally I'm not sure the control is actually needed. Some kind of |
|
Ok, I'm going to close this, thanks for your work on it! |
Closes #205