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

Fix vertical selection of neighbors #152

Open
wants to merge 3 commits into
base: v0.1.5-develop
Choose a base branch
from

Conversation

PabloLec
Copy link
Contributor

@PabloLec PabloLec commented Oct 16, 2021

Quick summary of pull request...

  • I have read the contribution guidelines
  • CI Unit tests pass
  • New functions/classes have consistent docstrings

What this pull request changes

This PR only modifies the _get_vertical_neighbors method for PyCUI object.
And... (sorry for that, I just noticed it) a few auto removal of useless whitespaces, sorry for the commit readability.

This is about issue #151.
I modified the method in order to:

  1. Get row scope to consider (UP) from current row to 0 or (DOWN) for current row to max row.
  2. Iterate through rows in that scope.
  3. For each row, get row items and ignore non selectable items.
  4. Sort row items by absolute column distance. In order to, e.g we are on column 8, we probably want to jump down to next item below on column 7 rather than item on column 1.
  5. Populate this neighbor list, and return this list of ids just as before.

Issues fixed with this pull request

Optionally, what changes should join this PR

  • I didn't test it but I think left and right arrow key navigation also allow to select non selectable widgets.

  • By the way, noticing my auto removal of useless whitespaces, I'd like to refactor some lines in py_cui, like unnecessary nesting, casting, etc. So, not replacing explicit code by unreadable smart code but essentially shorten some small parts in order to improve readability.
    The thing is I see multiple possible lines I could refactor in the whole codebase. I could do it and make a PR of a few commits, not a monstrosity, probably a diff of around 150 lines.
    I think it would be a good idea, I'm just wondering if the timing is good or if you're working on something right now and prefer to avoid blocking merges.

@PabloLec
Copy link
Contributor Author

I just updated RecoverPy demo GIF too. Better quality + I make a better use of PyCUI. Better showcase of PyCUI overall 😉

@PabloLec PabloLec changed the base branch from v0.1.4-develop to v0.1.5-develop October 29, 2021 21:37
@voryzen
Copy link
Contributor

voryzen commented Dec 30, 2021

Hey @PabloLec,
Does this need any testing to be done?

@PabloLec
Copy link
Contributor Author

PabloLec commented Jan 5, 2022

Hi! Sorry for the delay, the fix did work so no further testing needed but the same fix needs to be applied to horizontal selection though.

@voryzen
Copy link
Contributor

voryzen commented Jan 6, 2022

OK, no worries @PabloLec
Just wondering if I could help.

Fixing the horizontal and vertical nav would be a game changer for me @jwlodek

@jwlodek
Copy link
Owner

jwlodek commented Jan 17, 2022

@PabloLec Fixed some merge conflicts created by merging the formatting PR, but not 100% sure if the expected behavior is retained. Could you please double check? Also, if you could add some additional unit tests that account for the newly fixed vertical selection that would be great - basically just adding a new test file that tests vertical selection as it is expected to behave, in cases of successful upwards, successful downwards, unsuccessful downwards, unsuccessful upwards, and bottom/top of the UI as well.

@voryzen
Copy link
Contributor

voryzen commented Jan 17, 2022

Hi @PabloLec and @jwlodek,

Good to know your still around @jwlodek

@PabloLec
it should be noted that the new navigation, does not factor in 'unselectable' widgets that separate selectable widgets.

for example:
widget_a at 1,1 (selectable),
widget_b at 1,2 or 2,1 (unselectable), and
widget_c at 1,3 or 3,1 (selectable)

when attempting to navigate using the new code, the user will not be able to move from widget_a to widget_c, using the arrow keys.

I gather this has something to do with line 1133
return neighbors[0]
in the _check_if_neighbor_exists function.

I have yet to work out how to rectify this. any thoughts @PabloLec.

I also note that the cycle widgets function does not respect unselectable widgets either, even after implementing the new navigation code. Any ideas here would be appreciated, I think I might have a way to do it, but I won't get to it, for another few days, due to study commitments. Any ideas here would be welcome. Fixed in PR #171

@voryzen
Copy link
Contributor

voryzen commented Jan 25, 2022

@PabloLec I tried to fix the horizontal navigation, but it doesn't seem to work.
I will note though, that vertical navigation does respect the 'unselectable' widgets. all good there

@voryzen
Copy link
Contributor

voryzen commented Feb 12, 2022

hey @PabloLec @jwlodek any updates on this?

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.

None yet

3 participants