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

Winbar Support + Notes and Discussions; Help Popup + Auto-Open #133

Merged
merged 35 commits into from
Dec 13, 2023

Conversation

harrisoncramer
Copy link
Owner

@harrisoncramer harrisoncramer commented Dec 5, 2023

This MR changes the discussion view to have two different "tabs" that the user can switch between, one that contains notes and one that contains discussions.

The split will automatically open when the user opens the reviewer. The MR also splits apart the discussions module further by breaking diagnostics into their own module; adds a "help" popup for users to be able to see the local keybindings for the different parts of the plugin.

@harrisoncramer harrisoncramer marked this pull request as ready for review December 9, 2023 18:49
@harrisoncramer harrisoncramer changed the title Proof of Concept: Adding Winbar Winbar Support + Notes and Discussions; Help Popup + Auto-Open Dec 9, 2023
@jakubbortlik
Copy link
Collaborator

Hi, @harrisoncramer a lot of good stuff has been added in this MR! Here are some notes from trying it out. Though, I've only tried it out with a dummy MR, I'll see how it works when I try it at work.

  • Showing the discussions as the default view suits my needs, but other people might prefer to see notes by default. Would it be possible to make this configurable with something like { discussion_tree = { default_view = "discussions" }} -- one of "discussions", "notes"? Or maybe even enable the original split view with both discussions and notes?
  • With this is connected the next idea: When I select the notes view, toggle off discussions and toggle them on again, I'd expect the notes to be shown again, but instead I always get the discussions. Would it be possible to always show the view that was used last (or the default of course, when opening for the first time)? Or would it be even possible to make this configurable with something like on_toggle = "show_default_view" or on_toggle = "show_previous_view"? I'm not sure this would actually be useful, I'd know after some time of using it in practice.
  • When only one of discussions or notes is shown, the information about the other type is missing in the UI. Maybe it would possible to add the information in the winbar, something like Discussions (0/1 resolved) [Notes (0/7 resolved)] the whole square brackets would additionally be in a less prominent shade. An overview of notes and discussions could also be added into the Details section of the Summary window.
  • The discussions tree is not refreshed after adding a new comment, but the winbar is refreshed, so after adding the first comment I see this:
Discussions (0/1 resolved)
  1   No Discussions for this MR

This is fixed after toggling the tree. With notes not even the winbar is automatically refreshed.

  • The winbar is not refreshed after deleting a note or comment. Also some of the diagnostics are not cleared after deleting the respective comment. This is fixed after restarting nvim.
  • Sometimes the last comment in the list is not deleted and I get the following error message: gitlab.nvim: Could not delete comment: DELETE https://gitlab.cloud.............../merge_requests/1/discussions/0b12f59bba914c1fd3407b518fe75fb373cea7aa/notes/87034: 404 {message: 404 Not found}. After restarting nvim, the comment can be deleted without problems.
  • Each time I toggle the discussions, I see the message: "Loading discussions...". It takes about a second to compete, and thought this is not a big deal, I'm wondering whether it is something that could be fixed - this didn't seem to happen in the original split view. But maybe it's just that in the original view the whole split was loaded only after the discussions had been loaded and the lag was just not that noticeable because there was no message like "loading discussions..." :)

This seems like I have a lot of issues with your plugin, so I would like to stress that I am very grateful for your work! Having a customizable UI for Gitlab merge requests inside nvim is such a great thing! I would like to help more making it even better if I could, even if my ability to write Lua code is limited. I believe I could do small stuff like adding notes and discussions to the Summary window, if you decide it's something you'd like to have in your plugin.

@harrisoncramer
Copy link
Owner Author

harrisoncramer commented Dec 10, 2023

Thank you for all the helpful feedback!

  1. It's now configurable to open notes or discussions by default
  2. Persisting the selected view between opening and closing is kind of difficult and could potentially be done, but not something I'm going to attempt in this MR.
  3. I've adjusted the logic in the winbar to always show the titles for both views and highlight the currently active one. You can switch between them.
  4. I've fixed issues with deletions and other stuff so that the winbars and trees are always refreshed correctly.
  5. The deletion of the last comment error you pointed out may be an unrelated bug but it should be tracked separately
  6. The loading text is new. I've added it because I'd like to open the discussions view right away (we weren't doing this before) and giving the user feedback on the status of the API call is helpful.

I've pushed up all these changes to this branch, I'll be using this branch for a few days as well and unless I notice any more issues I'll merge this in this week. This is probably going to be a minor version bump because it changes the default behavior of the plugin slightly.

configured-winbar.mov

@jakubbortlik
Copy link
Collaborator

jakubbortlik commented Dec 11, 2023

Hi, @harrisoncramer thanks for the quick update! It all looks very good. I'll be testing it this week with actual MRs.

I've created one small cosmetic PR based on the adding-winbar branch, that fixes some issues I had with the Help popup.

and string.format("Notes (%d/%d)", t.resolved_notes, t.resolvable_notes)
or "Notes"

print(t.name)
Copy link
Collaborator

@jakubbortlik jakubbortlik Dec 11, 2023

Choose a reason for hiding this comment

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

Is this print call intentional or a leftover from development?

@jakubbortlik
Copy link
Collaborator

I've encountered a bug with the winbar. When I toggle off the discussions tree and then press <tab> on the DiffviewFilePanel to open the diff for the next file, I'm getting the following error:

Error executing vim.schedule lua callback: vim/_options.lua:0: Expected lua string
stack traceback:
        [C]: in function 'nvim_get_option_info'
        vim/_options.lua: in function '__index'
        vim/_options.lua: in function ''
        vim/_options.lua: in function '__index'
        ...zy/gitlab.nvim/lua/gitlab/actions/discussions/winbar.lua:55: in function 'update_winbar'
        ...lazy/gitlab.nvim/lua/gitlab/actions/discussions/init.lua:68: in function 'callback'
        ...lazy/gitlab.nvim/lua/gitlab/actions/discussions/init.lua:44: in function 'callback'
        ...ub/.local/share/nvim/lazy/gitlab.nvim/lua/gitlab/job.lua:40: in function ''
        vim/_editor.lua: in function ''
        vim/_editor.lua: in function <vim/_editor.lua:0>

@harrisoncramer
Copy link
Owner Author

Thank you, fixed. Will not attempt to redraw the winbar when the split is closed; also merged in your changes.

@jakubbortlik
Copy link
Collaborator

Thank you, fixed. Will not attempt to redraw the winbar when the split is closed; also merged in your changes.

Thanks. And sorry for that typo ("creape_popup_state"), I wonder why on earth I changed that word...

@harrisoncramer harrisoncramer merged commit d5510f9 into main Dec 13, 2023
6 checks passed
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.

2 participants