ComboBox multiple fixes #710

fergusonr · 2020-06-20T08:37:41Z

FIxes for #709

Dim.Fill() incorrectly calculated
List height not resizing
SetSource() not immediately updating list.
Double click not selecting item

…ht not resizing. SetSource() not immeadiately updating list. Double click not selecting item. Example now demo resizes with view

BDisp · 2020-06-20T09:27:37Z

@fergusonr, provide a functional constructor for the AllViewsTester.cs. Now there is an error on the searchset variable is null in this method.


		private int CalculatetHeight ()
		{
			var h = (Height is Dim.DimAbsolute) ? height : Bounds.Height;
			return Math.Min (Math.Max (0, h - 1), searchset.Count);
		}

In the Demo.cs the ListView is not closing on exit.

fergusonr · 2020-06-20T09:51:10Z

Thanks. Fixed AllViewsTester.cs. Investigating Demo.cs issue....

BDisp · 2020-06-20T09:59:15Z

Thanks. Is possible you have the default constructor to accept a ustring and using that to have a search and source filled with that information making visible? It's the way that AllViewsTester.cs work. See the CheckBox example.

BDisp · 2020-06-20T11:57:32Z

Thanks. It's possible to have that text also into the source? May be this? Not tested.


		public ComboBox () : this (string.Empty) { }

		/// <summary>
		/// Public constructor
		/// </summary>
		/// <param name="text"></param>
		public ComboBox (ustring text) : base ()
		{
			var source = new List<string> () { text.ToString () };
			search = new TextField (text.ToString ());
			listview = new ListView (source) { LayoutStyle = LayoutStyle.Computed, CanFocus = true };

			Initialize ();
			Text = text;
			SetSource (source);
		}

fergusonr · 2020-06-20T12:23:29Z

The text (what your searching for) is independent to the source (your searching in)

I think this, added to AllViewTesters.cs CreateClass() would be better for the IUCatalog, but it does not seem to initialize a ListView or ComboBox. Even though I can breakpoint in SetSource() and see its being set.

if(view.GetType().GetMethod("SetSource") != null) {
	IList<string> list = new List<string>() { "Test Text1", "Test Text2", "Test Text3" };
	view?.GetType ().GetMethod ("SetSource")?.Invoke (view, new object [] { list });
}

BDisp · 2020-06-20T12:30:44Z

I think you are wright. It seems it's not initializing in the ListView which must be fixed first. I also think that adding a Text property in the ListView with the default string selected would do (may be) the trick.

fergusonr · 2020-06-20T14:32:38Z

I can now get ListView and ComboBox scenario lists to initialize with this code change in addition to the one mentioned above in AllViewTesters.cs CreateClass

Need @tig to comment on the validity of making this change

	// Instantiate view
	var view = (View)Activator.CreateInstance (type);

	view.Width = Dim.Fill (); // <-- Added
	view.Height = Dim.Fill (); // <-- Added

BDisp · 2020-06-20T14:35:55Z

I will submit a PR to the ListView related and then with that please try if it could have a default search with the default source based on that search.

fergusonr · 2020-06-20T16:40:36Z

Not sure why you need the constructor your asking for? It has no use outside of testing. The same can be achieved with this:-

var combo = new ComboBox ("Test Text");
combo.SetSource(new List<string>() { "Test Text" });

BDisp · 2020-06-20T16:50:14Z

I understand your point of view but when @tig insisted on default builders I didn't immediately understand his intention. Check my PR #712 and you will see that even having only one default driver with a simple text it fills the ListView with the generic application AllViewTesters.cs. For now it only works with the Text and Title properties but @tig has already mentioned that it intends to implement other features.

tig · 2020-06-20T17:01:56Z

Need @tig to comment on the validity of making this change

	// Instantiate view
	var view = (View)Activator.CreateInstance (type);

	view.Width = Dim.Fill (); // <-- Added
	view.Height = Dim.Fill (); // <-- Added

I'd rather not add code like this to AllViewsTester, because one of the values of this Scenario is to illustrate the default behavior of views (to expose bugs).

tig · 2020-06-20T17:04:05Z

IMO, given the code I just pushed in #713, when ComboBox is selected, it should show "Test Text" in the TextField and List Item #1 (2 & 3) in the List. It does not do that now.

BDisp · 2020-06-20T17:04:27Z

Of course it is just a record but it follows the same principle as the others. Now reading a Source property as @tig intends to implement and not a method as you suggested (although it was also a good idea, don't misinterpret) is more appropriate for the purpose of this class that intends to test its properties.

BDisp · 2020-06-20T17:10:23Z

I am checking in the image I sent that the ListView does not keep the focus on the current selected item. @tig could fix this, please :-)

…ht not resizing. SetSource() not immeadiately updating list. Double click not selecting item. Example now demo resizes with view

…into combobox_fixes2

fergusonr · 2020-06-21T06:26:19Z

IMO, given the code I just pushed in #713, when ComboBox is selected, it should show "Test Text" in the TextField and List Item #1 (2 & 3) in the List. It does not do that now.

Thanks @tig Your not seeing the list for three reasons:-

1 There's a fix in this PR required
2 The "Test Text" does not match anything in the source list items

Change the list items to this:-

var source = new ListWrapper (new List<ustring> () { 
  ustring.Make("Test Text #1"), ustring.Make("Test Text #2"), ustring.Make("Test Text #3") });

3 The scenario environment is not fully emulating the real environment. Application.Loaded is not getting fired. Need this at the end of AllViewsTester.CreateClass()

View CreateClass (Type type) {
    ....
    ....
    ....
    Application.Loaded?.Invoke (new Application.ResizedEventArgs () { 
        Rows = Application.Driver.Rows, Cols = Application.Driver.Cols });

    return view;
}

There is an issue in that the list does not appear until you give / remove focus on the Combo. Not clear where the problem lies at the moment.

tig · 2020-06-21T15:13:06Z

Application loaded is not the right event. Use LayoutComplete.

…

-cek Sent from mobile device. Expect brevity, typos, and dangerous driving.

________________________________ From: Ross Ferguson <[email protected]> Sent: Saturday, June 20, 2020 11:26:29 PM To: migueldeicaza/gui.cs <[email protected]> Cc: Charlie Kindel ([email protected]) <[email protected]>; Mention <[email protected]> Subject: Re: [migueldeicaza/gui.cs] ComboBox multiple fixes (#710) IMO, given the code I just pushed in #713<#713>, when ComboBox is selected, it should show "Test Text" in the TextField and List Item #1 (2 & 3) in the List. It does not do that now. Thanks @tig<https://github.com/tig> Your not seeing the list for three reasons:- * 1 There's a fix in this PR required * 2 The "Test Text" does not match anything in the source list items Change the list items to this:- var source = new ListWrapper (new List<ustring> () { ustring.Make("Test Text #1"), ustring.Make("Test Text #2"), ustring.Make("Test Text #3") }); * 3 The scenario environment is not fully emulating the real environment. Application.Loaded is not getting fired. Need this at the end of AllViewsTester.CreateClass() View CreateClass (Type type) { .... .... .... Application.Loaded?.Invoke (new Application.ResizedEventArgs () { Rows = Application.Driver.Rows, Cols = Application.Driver.Cols }); return view; } [UICatalog ComboBox]<https://user-images.githubusercontent.com/17233550/85218217-02b06a80-b390-11ea-9d74-90b0148b386b.JPG> There is an issue in that the list does not appear until you give / remove focus on the Combo. Not clear where the problem lies at the moment. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#710 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAEO6CWHP4ZWJM6NV5OVIXDRXWRZLANCNFSM4ODJI5AQ>.

BDisp · 2020-06-21T21:57:58Z

@fergusonr can you add a down arrow that will open or close the list items of the ComboBox, please? In that case both the TextField and the ListView will be with the same width, that is Width - 1. Something like this.

		public override void Redraw (Rect bounds)
		{
			base.Redraw (bounds);
			Move (Bounds.Right - 1, 0);
			Driver.AddRune (Driver.DownArrow);
		}

		public override bool MouseEvent (MouseEvent me)
		{
			if (me.X == Bounds.Right - 1 && me.Y == Bounds.Top && me.Flags == MouseFlags.Button1Clicked) {
				// Process the drop-down.
				return true;
			}
			return base.MouseEvent (me);
		}

Terminal.Gui/Views/ComboBox.cs

tig

This is great progress. Thanks a ton for all your hard work on this.

See my notes on using brackets for nested statements.

Also, ApplicationLoaded is not the right event. Use LayoutComplete.

BDisp · 2020-06-22T16:40:41Z

I am checking in the image I sent that the ListView does not keep the focus on the current selected item. @tig could fix this, please :-)

I already fixed this in the PR #719.

BDisp · 2020-07-02T15:59:57Z

@fergusonr I inserted a ScrollView in the ComboBox. I hope you like it.

tig · 2020-07-02T17:55:22Z

Can one of you articulate a plan for finalizing this PR and ensuring all of the feedback has been incorporated? It's not clear to me how this will all come together.

Also, I'm not sure if this is addressed here yet or not, but it should be:

In Unicode.cs I had to comment out Combobox because of a crash due to Unicode handling:

BDisp · 2020-07-02T18:20:18Z

You can test my branch here (https://github.com/BDisp/gui.cs/tree/combobox-multiple-fixes) but @fergusonr will make your final decision as the author of the control. I just insisted that it is a ComboBox with AutoComplete features when typing but it will also have to work like a real ComboBox. But @fergusonr has every right to give his opinion and I am willing to collaborate with him to achieve common goals. Unicode already works because @fergusonr presented resolution in this PR.

Test dir more robust "\Windows" Use ListViewItemEventArgs

fergusonr · 2020-07-03T04:35:50Z

Yes, the Unicode issue was fixed a good while back in this PR.

var comboBox = new ComboBox ("gui.cs") {
	X = 20,
	Y = Pos.Y (label),
	Width = Dim.Percent (50),
	Height = Dim.Percent(20)
};

I believe this PR is ready to go. I've hand picked two of @BDisp changes. Any wider changes can be done in a new PR post v0.90 / v1.00. AutoHide for example should be made a public property so it can be overwritten

Note that PR #724 is in this branch

Thankyou both for all your help and patience

… combobox_fixes2

tig

This is really great work. You've simplified things greatly.

I appreciate your taking the feedback!

Terminal.Gui/Views/ComboBox.cs

tig · 2020-07-03T14:06:09Z

Terminal.Gui/Core/View.cs

@@ -596,6 +607,7 @@ public virtual void Remove (View view)
 			if (view == null || subviews == null)
 				return;

+			OnRemoving (view);


Please remove Removing from this PR. I have serious concerns about how it will interact with IDisposable and we don't have a clear use case where it's needed. I wish I could explain those concerns in detail, but I'm not confident I understand well enough yet. But I've learned to trust my instincts on things like this. There's no danger in removing the API for now. But there is danger in putting it in and having to change it later. Thanks.

Also, I am arguing for renaming Adding to Added. I made a comment that argues my point I think pretty clearly. I'll add to that argument that by using Adding (current tense) there's an implication that the operation can be cancelled. However, if we name it Added (past tense) it is very clear that the deal is done and there's no going back.

If you disagree, please explain. If you don't disagree, please rename to Added.

BDisp · 2020-07-03T14:26:29Z

I think there is no harm in maintaining OnRemoving. If it is to be removed then OnAdded must also be removed.

tig · 2020-07-03T14:43:37Z

I think there is no harm in maintaining OnRemoving. If it is to be removed then OnAdded must also be removed.

I think I've made my argument pretty clear. Beyond my concerns that the semantics around Removing will cause conflict with IDisposable, in framework/API design a key tenet to be followed is "Do not expose APIs that have no use cases." It just creates complexity and adds more code. History has shown that un-needed APIs just create bad legacy that has to be maintained in the future. We should strive to REMOVE code from the framework, not just add more and more.

If you feel strongly that Removing should be part of the API I would appreciate it if you explain why.

BDisp · 2020-07-03T20:27:05Z

If you feel strongly that Removing should be part of the API I would appreciate it if you explain why.

@tig if you prove it will create conflicts with the IDisposable implementation, I wouldn't think twice about supporting you. In this specific case of Removing I will tell you use cases in which they are very beneficial in implementing them. For example, ScrollView has two properties, namely ShowVerticalScrollIndicator and ShowHorizontalScrollIndicator, where they dynamically add or remove views from his control, which are the cases of the vertical and horizontal ``ScrollBarView. Now imagine that you create a control that includes the ScrollView as a subview and you want, for example, to resize any subview or other control as AutoHideScrollBars adds or removes the ScrollBarViews. If you had an event that notifies the subscribers of this occurrence, it becomes easier to react according to the needs.

BDisp · 2020-07-03T23:02:10Z

@fergusonr can you take a look for this small adjustments I have made to improve the ComboBox, please? Is more to hide the list if the ComboBox loses focus. Thanks.

https://github.com/BDisp/gui.cs/tree/pr%23710-combobox-multiple-fixes

… combobox_fixes2

@BDisp

SelectedItem (read only) from @BDisp OpenSelectedItem

…ion.

BDisp · 2020-07-06T09:34:07Z

@fergusonr, please merge my PR #723 again because I changed the Removing event to Removed to take action when the view no longer exists in the collection.

After selecting an item, the ComboBox closes but no longer opens either by clicking on the down arrow or with the cursor down.

BDisp · 2020-07-06T09:46:10Z

When autohide is true the ComboBox should close the list when it loses focus.

BDisp · 2020-07-06T10:32:25Z

With the mouse, the ComboBox should only get the focus with a click and not just passing the mouse over it.

…/gui.cs into combobox_fixes2

ComboBox multiple fixes. Dim.Fill() incorrectly calculated. List heig…

2833564

…ht not resizing. SetSource() not immeadiately updating list. Double click not selecting item. Example now demo resizes with view

fergusonr changed the title ~~ComboBox multiple fixes #709~~ ComboBox multiple fixes Jun 20, 2020

Fixed AllViewsTester.cs crash

a869b9c

Add constructor that takes a text argument

e164ca2

fergusonr added 6 commits June 20, 2020 18:28

ComboBox multiple fixes. Dim.Fill() incorrectly calculated. List heig…

cfcf8ef

…ht not resizing. SetSource() not immeadiately updating list. Double click not selecting item. Example now demo resizes with view

Fixed AllViewsTester.cs crash

f3b1b07

Add constructor that takes a text argument

6f97585

Accomodate upstream changes

acacd0c

Merge branch 'combobox_fixes2' of https://github.com/fergusonr/gui.cs …

4938566

…into combobox_fixes2

Fix unit test

4df3c4e

tig reviewed Jun 22, 2020

View reviewed changes

Terminal.Gui/Views/ComboBox.cs Outdated Show resolved Hide resolved

tig requested changes Jun 22, 2020

View reviewed changes

ComboBox. Allow user to set ColorScheme

f722f96

ComboBox hand pick @BDisp changes:-

de5a30b

Test dir more robust "\Windows" Use ListViewItemEventArgs

Merge branch 'master' of https://github.com/migueldeicaza/gui.cs into…

c71fad0

… combobox_fixes2

fergusonr requested a review from tig July 3, 2020 04:56

tig requested changes Jul 3, 2020

View reviewed changes

fergusonr added 2 commits July 4, 2020 01:28

Merge branch 'master' of https://github.com/migueldeicaza/gui.cs into…

8524009

… combobox_fixes2

ComboBox. Use Clear() instead of Redraw()

670595d

BDisp mentioned this pull request Jul 4, 2020

ComboBox source is IList but can only be list of strings #746

Closed

fergusonr and others added 4 commits July 5, 2020 01:50

ComboBox. Adhere to ListView interface, added:-

b9c8758

SelectedItem (read only) from @BDisp OpenSelectedItem

Changed to removed because it is safer to perform actions after delet…

e8972f4

…ion.

Fixing the Removed test.

e0c6067

ComboBox. Wire-up Home/End keys down to listview

d464525

fergusonr mentioned this pull request Jul 6, 2020

ComboBox focused last #742

Closed

fergusonr added 2 commits July 6, 2020 13:05

ComboBox. Fix #742

3800a23

Merge branch 'added-removing-view-events' of https://github.com/BDisp…

7d8f3ca

…/gui.cs into combobox_fixes2

tig approved these changes Jul 6, 2020

View reviewed changes

tig merged commit 98e224a into gui-cs:master Jul 6, 2020

tig mentioned this pull request Jul 6, 2020

ComboBox issues #709

Closed

fergusonr referenced this pull request in BDisp/Terminal.Gui Jul 6, 2020

Merge branch 'enter-leave-events' into pr/710-combobox-multiple-fixes

39b26cf

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ComboBox multiple fixes #710

ComboBox multiple fixes #710

fergusonr commented Jun 20, 2020 •

edited

Loading

BDisp commented Jun 20, 2020

fergusonr commented Jun 20, 2020

BDisp commented Jun 20, 2020

BDisp commented Jun 20, 2020 •

edited

Loading

fergusonr commented Jun 20, 2020

BDisp commented Jun 20, 2020

fergusonr commented Jun 20, 2020

BDisp commented Jun 20, 2020

fergusonr commented Jun 20, 2020 •

edited

Loading

BDisp commented Jun 20, 2020

tig commented Jun 20, 2020

tig commented Jun 20, 2020

BDisp commented Jun 20, 2020

BDisp commented Jun 20, 2020

fergusonr commented Jun 21, 2020

tig commented Jun 21, 2020 via email

BDisp commented Jun 21, 2020

tig left a comment

BDisp commented Jun 22, 2020

BDisp commented Jul 2, 2020

tig commented Jul 2, 2020

BDisp commented Jul 2, 2020

fergusonr commented Jul 3, 2020 •

edited

Loading

tig left a comment

tig Jul 3, 2020 •

edited

Loading

BDisp commented Jul 3, 2020

tig commented Jul 3, 2020

BDisp commented Jul 3, 2020

BDisp commented Jul 3, 2020

BDisp commented Jul 6, 2020

BDisp commented Jul 6, 2020

BDisp commented Jul 6, 2020

ComboBox multiple fixes #710

ComboBox multiple fixes #710

Conversation

fergusonr commented Jun 20, 2020 • edited Loading

BDisp commented Jun 20, 2020

fergusonr commented Jun 20, 2020

BDisp commented Jun 20, 2020

BDisp commented Jun 20, 2020 • edited Loading

fergusonr commented Jun 20, 2020

BDisp commented Jun 20, 2020

fergusonr commented Jun 20, 2020

BDisp commented Jun 20, 2020

fergusonr commented Jun 20, 2020 • edited Loading

BDisp commented Jun 20, 2020

tig commented Jun 20, 2020

tig commented Jun 20, 2020

BDisp commented Jun 20, 2020

BDisp commented Jun 20, 2020

fergusonr commented Jun 21, 2020

tig commented Jun 21, 2020 via email

BDisp commented Jun 21, 2020

tig left a comment

Choose a reason for hiding this comment

BDisp commented Jun 22, 2020

BDisp commented Jul 2, 2020

tig commented Jul 2, 2020

BDisp commented Jul 2, 2020

fergusonr commented Jul 3, 2020 • edited Loading

tig left a comment

Choose a reason for hiding this comment

tig Jul 3, 2020 • edited Loading

Choose a reason for hiding this comment

BDisp commented Jul 3, 2020

tig commented Jul 3, 2020

BDisp commented Jul 3, 2020

BDisp commented Jul 3, 2020

BDisp commented Jul 6, 2020

BDisp commented Jul 6, 2020

BDisp commented Jul 6, 2020

fergusonr commented Jun 20, 2020 •

edited

Loading

BDisp commented Jun 20, 2020 •

edited

Loading

fergusonr commented Jun 20, 2020 •

edited

Loading

fergusonr commented Jul 3, 2020 •

edited

Loading

tig Jul 3, 2020 •

edited

Loading