Skip to content
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

use mouse down for activation of components #488

Merged
merged 25 commits into from
Jun 14, 2020

Conversation

ginkoblongata
Copy link
Contributor

Seeking comment.

Looks like the code was taking a direction that I'm not sure I understand the rationale, it would take two clicks for things, one for the focus and another for the activation.

Since mouse is newer terminals, not sure the case where this would be needed or desired.

This pull request collapses those, so clicking a button once causes the action. Clicking a checkbox once causes the focus and the checked state to toggle.
Clicking the RadioButton causes the focus and that button state to be true, etc.

@@ -173,7 +173,7 @@ public synchronized Result handleKeyStroke(KeyStroke keyStroke) {
* @return index of a item that was clicked on with {@link MouseAction}
*/
protected int getIndexByMouseAction(MouseAction click) {
return click.getPosition().getRow() - getGlobalPosition().getRow();
return Math.min(click.getPosition().getRow() - getGlobalPosition().getRow(), items.size() -1);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When a list has the screen real estate to present more items than the list actually has in it, there will be blank space at the bottom rows.
This change makes it so that clicking in that blank space selects the last item rather than throwing an exception.

@avl42
Copy link
Contributor

avl42 commented Jun 7, 2020

I think, it makes sense to let certain widgets already do their action on first click, and depending on the widget it isn't even necessary to set focus on it at all, so let the "set focus" be the default (for interactive widgets), and for specifc widgets either override it for a specific action (buttons, checkboxes), or just continue having them focused (text boxes, although they could in future also set the cursor to (or nearest to) the clicked position, which is quite non-trivial to implement.)

There might also be specific widgets, that may still use first click to focus them, and only every further click (i.e. a click, while they're already focused) to trigger further action. That would be widgets that would be primarly keyboard-controlled, so a user could click on it for focus and then continue with keyboard, without having that click already do anything else - TextBox would be an example for that, in my humble opinion.

@ginkoblongata
Copy link
Contributor Author

Seeking coment:

For certain components, doing the action right away on mouse click is good.
They still should also then be focused so that the keyboard interaction works seamlessly.
I believe that I have done that approach across the widgets in this pull request.

Looks like space and enter were 'actioning' keys on some components, some others were only enter so I made them have space as well for consistency.

I also added some clipping logic to prevent some exceptions when the mouse event locations would map to imaginary items in the lists and tables and text boxes.

For the table test, I needed to have it rendered for the accurate clipping log, seems to work on my machine (mvn clean install), not sure if popping up that swing window for couple milliseconds will be a build problem.

How would you like to proceed? I think that I am basically done with this issue.
There is another enhancement I had in mind but I think maybe it would be best to put into a new request. For the Menu, when the user clicks 'outside' it seems it should perform the logic which the ESC key performs as well.

@ginkoblongata
Copy link
Contributor Author

Looks like there is still a thing left to do. The mouse down is not taking into account being scrolled downward in TextBox, ActionListBox, CheckBoxList, RadioBoxList.

@ginkoblongata
Copy link
Contributor Author

Ok, looks like it's a wrap, got the mouse down working for list style widgets.

I am wondering how to proceed to have this pull request merged. I was thinking it would be best for it to merge before proceeding onto other things so it doesn't topple.

…ate test.jar in order to run tests from command line
@mabe02
Copy link
Owner

mabe02 commented Jun 14, 2020

Could you change this PR to go against release/3.1 branch instead of master?

@ginkoblongata
Copy link
Contributor Author

Could you change this PR to go against release/3.1 branch instead of master?

Yes.

@ginkoblongata ginkoblongata changed the base branch from master to release/3.1 June 14, 2020 03:17
@mabe02 mabe02 merged commit 914dc1e into mabe02:release/3.1 Jun 14, 2020
@mabe02
Copy link
Owner

mabe02 commented Jun 14, 2020

Thanks, that's merged now

@ginkoblongata
Copy link
Contributor Author

Ok, great! Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants