-
-
Notifications
You must be signed in to change notification settings - Fork 703
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
Add a handler for ListView.SearchForVirtualItem in winforms backend for keyboard navigation in tables and detailed lists #2956
Changes from 1 commit
1593e6a
4902cb6
aa93bfd
6b969b4
1e16b9c
e498613
0ffe4ce
c7bae00
15f46f9
a35e255
59bee96
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
…le keyboard test
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -123,8 +123,11 @@ def winforms_search_for_virtual_item(self, sender, e): | |
while True: | ||
# Either winforms might provide a starting index out of bounds if searching at list borders, | ||
# or this loop may travel out of bounds itself while searching. In either case, wrap around. | ||
if i < 0: | ||
i = len(self._data) - 1 | ||
if i < 0: # pragma: no cover | ||
# This could theoretically happen if this event is fired with a backwards search direction, | ||
# however this edgecase should not take place within Toga's intended use of this event. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor style tweak - we try to keep comments to ~80 chars, allowing up to 88 at a stretch if it helps with flow. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry for the delay, this one is stumping me because I very rarely contribute to projects coded by sighted people and thus I haven't run into this restriction before. I'll work out a way to stylize comments such as by writing a program that reads clipboard input, trims all whitespace then reports on the line length so I know whether the comment needs further shortening without spamming the right arrow key 80 times and slowly counting, removing one word then doing it again etc. I won't be able to write comments for Toga's codebase until I've come up with an accessible solution or another blind person tells me one, and none I know who might know are around at the second. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry for double comment, just remembered to ask. Is this 80 chars per comment or 80 chars per line, I'm guessing per-comment? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The limit is 80 characters per line of code, including any leading space. It's essentially a holdover from old teletype programming - you should be able to see the entire line of code on an 80x25 text mode terminal screen, with no line wrapping. We've now got much larger screens that aren't constrained by 80 columns, but visual readability decreases with line length, so it's now a "soft 80" limit. The fact that this isn't an consideration for non-sighted programmers is a fascinating insight - thanks for letting me know about that. As far as automated tooling goes - most sighted IDEs (e.g., Visual Studio) can do an "auto word-wrap" on comments; I'm not sure what your tools will allow. At the very least, the flake8 configuration should be catching this, but I've just noticed that our flake8 configuration sets a line length of 119 (the config is in tox.ini for... reasons), so we're not flagging > 88 character lines. That's something we should probably fix - I've opened #2975 to track this improvement. In the meantime, I definitely don't want this to be an impediment to your ability to contribute to Toga. If you can't find an automated solution, I'm more than happy to do the last bit of leg work and re-flow long comments. I often end up tweaking comments in PRs anyway (purely for language style and content reasons), so it's not a major imposition. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought we standardized on 88 rather than 80, for the reasons explained here? It's better to be consistent, so we don't end up in the situation we had before when code was being reformatted every time it moved from one developer to another. In VS Code I use this extension. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The limit that is hard-enforced is definitely 88; I still think of it as a "soft 80". Make of that what you will 😀 And yes - I use that VSCode tooling as well; the issue is that the PR author is blind, and AFAIK isn't using Visual Studio. |
||
# i = len(self._data) - 1 | ||
raise NotImplementedError("backwards search unsupported") | ||
elif i >= len(self._data): | ||
i = 0 | ||
if ( | ||
|
@@ -135,8 +138,10 @@ def winforms_search_for_virtual_item(self, sender, e): | |
): | ||
found_item = True | ||
break | ||
if find_previous: | ||
i -= 1 | ||
if find_previous: # pragma: no cover | ||
# Toga does not currently need backwards searching functionality. | ||
# i -= 1 | ||
raise NotImplementedError("backwards search unsupported") | ||
else: | ||
i += 1 | ||
if i == e.StartIndex: | ||
|
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.
Is there a reason to use this call, rather than
self.widget.focus()
?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.
Yes, because as I'd previously mentioned, toga.Table.focus() is currently a no-op for some reason, it's been explicitly disabled in core/src/toga/widgets/table.py
I was planning to open an issue about this as I, as well, don't understand why table.focus has been made into a no-op, it makes it so I can't focus my users on a list programmatically which I wanted to do since a giant list was the primary control of my app.
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.
Ah - my apologies - I forgot you'd raised that.
I can't give you a good answer for why it's been disabled; my guess is that there might have been some odd behavior related to selection handling. That's definitely worth a standalone issue (and investigation); if you can fix it, that would definitely be welcome. I can't see any fundamental reason why a table shouldn't be able to be given focus, as long as the behaviour otherwise well defined.
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.
Also - that issue doesn't need to be fixed to land this PR; but it might be worth dropping a comment on the "pseudo-focus" statements highlighting that they could be replaced with
self.widget.focus()
once the issue is resolved (adding a reference to the new ticket number for that issue).