purebred-mua / purebred

A terminal based mail user agent based on notmuch
GNU Affero General Public License v3.0
139 stars 19 forks source link

Put an (invisible) cursor at the beginning of thread list items #436

Closed mlang closed 3 years ago

mlang commented 3 years ago

This is for the benefit of screen readers which use the cursor position as the primary focus indicator.

frasertweedale commented 3 years ago

The rest of the commit LGTM; the Nix test is failing but that's fine.

mlang commented 3 years ago

OK, import formatting (done with stylish-haskell) has been reverted. I basically just ran it because I was too lazy to manually find the places where to insert new imports such that they are sorted.

I was hoping to do a similar change (cursor) for ListOfMails, however, it appears there is a problem with where focus is, which I'd love to talk to you about. As I understand the code, focus is moved around when Up/Down are pressed in MailView, but ListOfMails does not have focus. So the simple check (ListOfMails == ^focusedViewWidget s) does not return True in MailView. I find the focus management of purebred a little confusing. Maybe you could advice on how to do a similar change to ListOfMails. ListOfThreads was easy, since the focus is indeed reported on ListOfThreads. However, MailView seems to keep the focus on ScrollingMailView, but the up/down keypresses are still rerouted to ListOfMails. In a sense, it would be easier if it were the other way around: Keep focus in ListOfMails, and reroute scrolling commands to the ScrollingMailView. So, any advice on putting a (invisible) cursor on the currently selected ListOfMails item?

romanofski commented 3 years ago

OK, import formatting (done with stylish-haskell) has been reverted. I basically just ran it because I was too lazy to manually find the places where to insert new imports such that they are sorted. I was hoping to do a similar change (cursor) for ListOfMails, however, it appears there is a problem with where focus is, which I'd love to talk to you about. As I understand the code, focus is moved around when Up/Down are pressed in MailView, but ListOfMails does not have focus. So the simple check (ListOfMails == ^focusedViewWidget s) does not return True in MailView. I find the focus management of purebred a little confusing. Maybe you could advice on how to do a similar change to ListOfMails. ListOfThreads was easy, since the focus is indeed reported on ListOfThreads. However, MailView seems to keep the focus on ScrollingMailView, but the up/down keypresses are still rerouted to ListOfMails. In a sense, it would be easier if it were the other way around: Keep focus in ListOfMails, and reroute scrolling commands to the ScrollingMailView. So, any advice on putting a (invisible) cursor on the currently selected ListOfMails item?

I think this is just a historic reason on how it was built, rather then a conscious choice.

I actually need to ask since I don't know how accessibility works in a terminal. Where ever the cursor is positioned, the screen reader will read this section? So for MailView it would be useful to read the selected entry and then the actual mail? It would probably be good to file an issue and we discuss this there?

romanofski commented 3 years ago

One small nit from hlint:

src/UI/Index/Main.hs:23:1-40: Warning: Use fewer imports
Found:
  import Brick.Types ( Padding(..), Widget )
  import Brick.Types ( Location(..) )
Perhaps:
  import Brick.Types ( Padding(..), Widget, Location(..) )

1 hint

and we can merge :)

frasertweedale commented 3 years ago

@mlang can you please squash your undo formatting commit into the preceding commit?

frasertweedale commented 3 years ago

@mlang alternatively, if @romanofski is happy with the change I can do the squash and push myself.

romanofski commented 3 years ago

Yeah and address the small hlint nit. Either way...

frasertweedale commented 3 years ago

Commit: https://github.com/purebred-mua/purebred/commit/a457b2d986a527312033966c80059580b26eb64f . Waiting for CI results. If good, I'll push it and close this PR.

frasertweedale commented 3 years ago

CI passed: https://github.com/purebred-mua/purebred/runs/3458958069 .

Pushed a457b2d986a527312033966c80059580b26eb64f to master.

Closing this PR.

@mlang big thanks for contributing!