feat(notifications): show and read unread notifications

[tris203]

Describe what this PR does / why we need it

Add ability to see unread notifications, review and mark as read from within octo

Does this pull request fix one issue?

Fixes #119

Describe how you did it

Use REST API for notifications as GraphQL is lacking

Describe how to verify it

1. :Octo notification
2. See unread notifications
3. scroll the list of PRs/Issues
4. Enter will open that issue/PR
5. <leader>nr will mark the notification as read

Special notes for reviews

Currently limited to unread notifications only due to API limitations
Icons are in place for unread, for extension in the future

Checklist

• Passing tests and linting standards
• Documentation updates in README.md and doc/octo.txt

[pwntester]

Thanks for the PR @tris203 This is great! I was waiting for GitHub to give us access to the more advanced notification implementation where you can mark them as done and read separately but it seems that is not going to happen anytime soon

[tris203]

Done and rebased @pwntester

[wd60622]

My telescope picker from the Octo is pretty tight. Can we make the first column width based on the width of the longest word (maybe with some padding)

[wd60622]

I tend to have use Octo from within the single repo, how about a configuration to filter to just the notifications for the current repo. Then no need to show the repo name as well. What are your thoughts?

[tris203]

I tend to have use Octo from within the single repo, how about a configuration to filter to just the notifications for the current repo. Then no need to show the repo name as well. What are your thoughts?

I don't think that particularly makes sense given the current API limitations. Given that we only show unread notifications, you would end up looking at an empty list 99% of the time.

Also a general, what do I have to do would mean pinging around all over the place to see where you might have notifications

[wd60622]

I tend to have use Octo from within the single repo, how about a configuration to filter to just the notifications for the current repo. Then no need to show the repo name as well. What are your thoughts?

I don't think that particularly makes sense given the current API limitations. Given that we only show unread notifications, you would end up looking at an empty list 99% of the time.

Also a general, what do I have to do would mean pinging around all over the place to see where you might have notifications

I was thinking pull all notifications but filter before passing to picker

[tris203]

I tend to have use Octo from within the single repo, how about a configuration to filter to just the notifications for the current repo. Then no need to show the repo name as well. What are your thoughts?

I don't think that particularly makes sense given the current API limitations. Given that we only show unread notifications, you would end up looking at an empty list 99% of the time.
Also a general, what do I have to do would mean pinging around all over the place to see where you might have notifications

I was thinking pull all notifications but filter before passing to picker

That works, but I don't know if with the telescope API if we can "mark an item read" without refreshing the whole list?

[wd60622]

That works, but I don't know if with the telescope API if we can "mark an item read" without refreshing the whole list?

But that affects even the case without the filter, right?

[tris203]

That works, but I don't know if with the telescope API if we can "mark an item read" without refreshing the whole list?

But that affects even the case without the filter, right?

No, because we just grab the unread items list, and marking it read fires the API even and optimistically removes it from the selector without the need to refresh the whole thing.

We could change it to a notification list and a notification list all or something similar, but I don't know how that best fits with the command list, and your still going to end up looking at an empty list most of the time.

[wd60622]

No, because we just grab the unread items list, and marking it read fires the API even and optimistically removes it from the selector without the need to refresh the whole thing.

Seems like the same logic for the case where the list is already filtered

We could change it to a notification list and a notification list all or something similar,

Would a configuration option not suffice?

but I don't know how that best fits with the command list, and your still going to end up looking at an empty list most of the time.

The other pickers have a "no matching ... found". Would a message like that be applicable here? What is the behavior with no notifications at the moment?

[tris203]

No, because we just grab the unread items list, and marking it read fires the API even and optimistically removes it from the selector without the need to refresh the whole thing.

Seems like the same logic for the case where the list is already filtered

We could change it to a notification list and a notification list all or something similar,

Would a configuration option not suffice?

Sure

but I don't know how that best fits with the command list, and your still going to end up looking at an empty list most of the time.

The other pickers have a "no matching ... found". Would a message like that be applicable here? What is the behavior with no notifications at the moment?

an empty list at the moment, i guess a no matching notifactions makes sense

Seeing as this is fully new functionality, IMO i think it makes sense to get a base design in and iterate on it later once it is being used.
At the moment, the state of this seems to work for my functionality of opening neovim and looking what needs to be done.
Or being working on something, seeing the email notification come in and opening the list in neovim without having to think about switching to the right project first

Also given that this is a bit of a stop gap until the "real" graphql endpoint eventually comes

[wd60622]

I think a rebase and address my comments would make this good to go

[wd60622]

Just one final suggestion on the spacing being too big

[wd60622]

Could @pwntester take another look. Your requested change is blocking a potential merge

[tris203]

I think your rebase has broken the icons too, but I will have a look

[wd60622]

I think your rebase has broken the icons too, but I will have a look

I did a rebase then fix for the icons. Take a look now. I had all items working