sindrets / diffview.nvim

Single tabpage interface for easily cycling through diffs for all modified files for any git rev.
Other
3.92k stars 107 forks source link

[Feature Request] DiffviewToggle command. #450

Closed singlexyz closed 8 months ago

singlexyz commented 9 months ago

Description

After opening Diffview, need to close it using :DiffviewClose or :tabclose.

This also implies that different methods are required to close Diffview, possibly involving additional keymaps.

If using the DiffviewToggle command, a single keymap can be set to either open or close Diffview.

Expected behavior

When Diffview open, run :DiffviewToggle to close it.

Actual behavior

There's no :DiffviewToggle command, had to run :DiffviewClose or :tabclose to close it.

Steps to reproduce

Not needed

Health check

Not needed

Log info

Not needed

Neovim version

Not needed

Operating system and version

MacOs

Minimal config

{
    "sindrets/diffview.nvim",
    keys = {
      { "<A-d>", "<cmd>DiffviewOpen<cr>" },
    },
    opts = {},
  },
jacobrreed commented 9 months ago

Ya would love the ability to map one keybind to toggle, or at the very least access a global variable to check myself and map accordingly

benwilliams140 commented 9 months ago

Working on something here, fairly simple and works pretty well.

miguno commented 8 months ago

+1, would love to see this

sindrets commented 8 months ago

Duplicate of #11. Rel. #162.

The concept of toggling tab pages doesn't exist anywhere else in vim. I'm not interested in having it here. In many scenarios the builtin mapping :h g<Tab> is functionally what you want, allowing you to jump back and fourth between the two most recently accessed tab pages.

jacobrreed commented 8 months ago

No, we're just asking for something like you storing a variable that toggles to show or hide plugin instead of assigning two keys for open and close

sindrets commented 8 months ago

@jacobrreed there's already t:diffview_view_initialized. You should be able to use that.

jacobrreed commented 8 months ago

@jacobrreed there's already t:diffview_view_initialized. You should be able to use that.

Ya I get it, just doesn't make sense for everyone using this plugin to write a keymap command function to do toggling logic based on that when you could add it for everyone I guess was the ask here

sindrets commented 8 months ago

@jacobrreed "everyone" do not have to do that. There's no need to create this arbitrary toggle keymap. If you instead create some convenient mappings to the builtin tab management commands (:tabn, :tabp, :tabc), those will provide useful far more generally. Not just specifically for the tab pages created by this plugin. I'm repeating myself ad nauseam here, I believe it's a misguided endeavour to try to create toggles for every single little thing, rather than just getting comfortable with tab/buffer/window management and navigation.

As I've also explained, it's not clear how exactly a toggle command should behave, given that the plugin encompasses multiple different tab page UIs, that each accept a number of options, and there may be multiple of them open at any given time. A toggle command would have to abide by a number of rather arbitrary rules. Which - in practicality for the user - means ambiguity and unexpected behavior. Which is unacceptable when it's entirely avoidable.

A "toggle" with ambiguous logic quite simply shouldn't exist. For a toggle to make sense there must be exactly two distinct states, where it's entirely clear that one can invert to the other. That's simply not the case here.

If you think you have a clear idea about how you wish this custom functionality to work in your own config; that's great! It should be simple to write some logic in a small function to achieve that. However, whatever you end up with: it wouldn't have been the right solution for everyone.

miguno commented 8 months ago

Hi all. First, thanks @sindrets for your continued work on this plugin. Much appreciated!

Can I make a suggestion?

It seems that some users of the plugin have a UX problem, and the proposed solution was to add a toggle feature like DiffviewToggle. Now, @sindrets is explaining his reasoning why that solution is not the best idea (see his comments above). Some alternative approaches were suggested instead, such as some custom Lua code in mentioned in https://github.com/sindrets/diffview.nvim/issues/11.

Another example:

If you instead create some convenient mappings to the builtin tab management commands (:tabn, :tabp, :tabc), those will provide useful far more generally.

See, that is indeed an option. However, I am not using (neovim's) tabs at all. Personally, I have a no need for using them because all my keybinds, my neovim, and my terminal are set up in a way that tabs are only marginally valuable for me. But that's just me. (I'm pretty confident that everyone in this thread likes neovim in particular because one can fine-tune it to one's personal taste and workflow habits.) Other users might indeed not know about neovim's tabs, so what @sindrets suggests would be much better indeed, because they can learn and use tab management not just for diffview, but also to improve their neovim experience in general.

A "toggle" with ambiguous logic quite simply shouldn't exist. For a toggle to make sense there must be exactly two distinct states, where it's entirely clear that one can invert to the other. That's simply not the case here.

Fully agree. The difference for this conversation is, however, that some users might not use multiple tab page UIs in diffview. My impression is that @sindrets use of diffview is much more involved and broad that I am personally using it, which is essentially a one-time opening of neogit/diffview to double-check what I am about to commit at the end of some coding work. (Similar to what I might do in a separate terminal with git directly. But sometimes I prefer to just stay within neovim for that.) In that use case, asking for a "toggle" makes sense for the user because it's indeed only a single page UI you will ever open or close. I only add this here because the conversation has become a bit heated, and one can see the different opinions are likely resulting from different vantage points ("I use neogit/diffview in a way that is different from yours.").

That out of the way, what about the following option: Update the diffview docs with information on how to get a "toggle" UX. First, this should include the info about tab management (see above and the linked GH issues). Second, this should include the orthogonal approach of adding custom Lua code for an actual "toggle" feature for those users that don't prefer the first way. Personally, I don't need an explicit DiffviewToggle command, I just want to use a single keybind to open/close the single "window" for diffview that I am using, as I find that more ergonomic myself (and because I am running out of available keybinds...).

Would that be acceptable?

My experience is that this would be a much more scalable solution (cf. "I'm repeating myself ad nauseam here") than addressing this particular user question individually every single time it pops up. I, for one, would be perfectly happy to add some custom Lua code to my config, and it would have been even better if there had been a ready-to-use code snippet in diffview's docs that I could have pasted into my neovim config.

Update: Here's the Lua snippet I am using for toggling diffview.

diffview_toggle = function()
  local lib = require("diffview.lib")
  local view = lib.get_current_view()
  if view then
    -- Current tabpage is a Diffview; close it
    vim.cmd.DiffviewClose()
  else
    -- No open Diffview exists: open a new one
    vim.cmd.DiffviewOpen()
  end
end
jacobrreed commented 8 months ago

Hi all. First, thanks @sindrets for your continued work on this plugin. Much appreciated!

Can I make a suggestion?

It seems that some users of the plugin have a UX problem, and the proposed solution was to add a toggle feature like DiffviewToggle. Now, @sindrets is explaining his reasoning why that solution is not the best idea (see his comments above). Some alternative approaches were suggested instead, such as some custom Lua code in mentioned in #11.

Another example:

If you instead create some convenient mappings to the builtin tab management commands (:tabn, :tabp, :tabc), those will provide useful far more generally.

See, that is indeed an option. However, I am not using (neovim's) tabs at all. Personally, I have a no need for using them because all my keybinds, my neovim, and my terminal are set up in a way that tabs are only marginally valuable for me. But that's just me. (I'm pretty confident that everyone in this thread likes neovim in particular because one can fine-tune it to one's personal taste and workflow habits.) Other users might indeed not know about neovim's tabs, so what @sindrets suggests would be much better indeed, because they can learn and use tab management not just for diffview, but also to improve their neovim experience in general.

A "toggle" with ambiguous logic quite simply shouldn't exist. For a toggle to make sense there must be exactly two distinct states, where it's entirely clear that one can invert to the other. That's simply not the case here.

Fully agree. The difference for this conversation is, however, that some users might not use multiple tab page UIs in diffview. My impression is that @sindrets use of diffview is much more involved and broad that I am personally using it, which is essentially a one-time opening of neogit/diffview to double-check what I am about to commit at the end of some coding work. (Similar to what I might do in a separate terminal with git directly. But sometimes I prefer to just stay within neovim for that.) In that use case, asking for a "toggle" makes sense for the user because it's indeed only a single page UI you will ever open or close. I only add this here because the conversation has become a bit heated, and one can see the different opinions are likely resulting from different vantage points ("I use neogit/diffview in a way that is different from yours.").

That out of the way, what about the following option: Update the diffview docs with information on how to get a "toggle" UX. First, this should include the info about tab management (see above and the linked GH issues). Second, this should include the orthogonal approach of adding custom Lua code for an actual "toggle" feature for those users that don't prefer the first way. Personally, I don't need an explicit DiffviewToggle command, I just want to use a single keybind to open/close the single "window" for diffview that I am using, as I find that more ergonomic myself (and because I am running out of available keybinds...).

Would that be acceptable?

My experience is that this would be a much more scalable solution (cf. "I'm repeating myself ad nauseam here") than addressing this particular user question individually every single time it pops up. I, for one, would be perfectly happy to add some custom Lua code to my config, and it would have been even better if there had been a ready-to-use code snippet in diffview's docs that I could have pasted into my neovim config.

Update: Here's the Lua snippet I am using for toggling diffview.

diffview_toggle = function()
  local lib = require("diffview.lib")
  local view = lib.get_current_view()
  if view then
    -- Current tabpage is a Diffview; close it
    vim.cmd.DiffviewClose()
  else
    -- No open Diffview exists: open a new one
    vim.cmd.DiffviewOpen()
  end
end

This is really all i wanted lol, this snippet but built into the plugin, ill just add it myself locally thats fine, I just dont understand the theory on not having a toggle function, but having an open and close command, but it's your plugin so be it

sindrets commented 8 months ago

@miguno: My impression is that @sindrets use of diffview is much more involved and broad that I am personally using it, which is essentially a one-time opening of neogit/diffview to double-check what I am about to commit at the end of some coding work. (Similar to what I might do in a separate terminal with git directly. But sometimes I prefer to just stay within neovim for that.) In that use case, asking for a "toggle" makes sense for the user because it's indeed only a single page UI you will ever open or close.

I disagree somewhat. It only makes sense from the user's perspective if - and only if - they don't know what vim tabs are. And then we're talking about users that aren't yet competent at using vim. I think it's wrong to make design decisions from that perspective.

That out of the way, what about the following option: Update the diffview docs with information on how to get a "toggle" UX. [...]

Absolutely. Improvements to documentation are always welcome.


@jacobrreed: [...] I just dont understand the theory on not having a toggle function, but having an open and close command, but it's your plugin so be it

You can't be serious. I just explained this exact thing in significant detail. At this point I have to assume that either your reading comprehension leaves something to be desired, or you're engaging in bad faith.

I will try one last time to explain, this time using an analogous example: a web browser also allows you to open and close browser tabs. Bookmarks allow you to open a tab to a specific webpage. Yet it doesn't offer a "toggle bookmark" functionality. What would a "toggle bookmark" button do? If a user has multiple tabs with the bookmark already open, and invokes the toggle; what happens? Should it open a new tab with the bookmark? Should it reuse one of the already open tabs? If so, which one? The first? The Last? The first accessed? The last accessed? Coming up with answers to these questions for yourself will not answer them for other users. This strange hypothetical functionality would inevitably lead to unexpected behavior. No, this is much too silly. There's an open action, there's a close action, and they are separate.

Again, I repeat myself: for a toggle to make sense there must be exactly two distinct states, where it's entirely clear that one can invert to the other. That's simply not the case here.

miguno commented 8 months ago

I created a pull request to document the "toggling" discussed here.

https://github.com/sindrets/diffview.nvim/pull/455