Add mapping to exit entire blame (#78)

bimlas commented 7 years ago

It's not binded to anything yet. To try it out, press q in any of the Gina blame windows:

map q <Plug>(gina-blame-exit)

bimlas commented 7 years ago

I think it would be nice to bind it to a default key (q), but I don't know that is this a good idea or not: the user cannot use macro recording. I think it's admissible, but I'm curious about your opinions.

prabirshrestha commented 7 years ago

Tried it out and seems to work. Would be good to have default mapping for quit.

autocmd! filetype gina-blame map q <Plug>(gina-blame-exit)

Unfortuantely his will only work for the left side but not right side (code) but this would still be better than nothing.

lambdalisue commented 7 years ago

Thanks for the PR 👍

I think it would be nice to bind it to a default key (q), but I don't know that is this a good idea or not: the user cannot use macro recording. I think it's admissible, but I'm curious about your opinions

I think it should NOT be binded in default while

As you said, it kill macro recording
It is easy to define by user

Unfortuantely his will only work for the left side but not right side (code) but this would still be better than nothing.

The filetype autocmd does not suite for this purpose while the filetype of the right side is not gina-blame. You should read a custom mapping section on gina's document for detail.

bimlas commented 7 years ago

I didn't changed bwipeout yet, because I cannot use buffer names on quit, nor close. What do you like to be if there are only the blame buffers exists: quiting Vim, or opening an empty buffer? I think quiting is better, because if there are only 2 buffers exists, that means the user opened blame in an unusual way (opening from an existing buffer means that the original buffer exists after quiting blame).

bimlas commented 7 years ago

Note that quit triggers QuitPre autocommands which may lead to bugs, close fails if only one window exists. I think bwipeout is okay, since bufhidden=wipe wipes out anyway if the buffer gets hidden.

lambdalisue commented 7 years ago

I'm sorry for the late reply. I have not get stable internet connection yet...

since bufhidden=wipe wipes out anyway if the buffer gets hidden.

Well that's why I don't want to use bwipeout directly. I would make it possible to control the behavior by changing bufhidden options.

because I cannot use buffer names on quit, nor close

Use execute 'quit' bufnr(bufname) or execute 'close' bufnr(bufname) instead.

What do you like to be if there are only the blame buffers exists

How about to make two mappings, say <Plug>(gina-blame-quit) and <Plug>(gina-blame-close) and leave both for user's choice?

bimlas commented 7 years ago

:quit (<Plug>(gina-blame-quit)) works, but it's closing Vim, not just the blame view. :close (<Plug>(gina-blame-close))prints E444: Cannot close last window

diff --git a/autoload/gina/command/blame.vim b/autoload/gina/command/blame.vim
index 3ee0101..638de47 100644
--- a/autoload/gina/command/blame.vim
+++ b/autoload/gina/command/blame.vim
@@ -61,8 +61,10 @@ function! s:call(range, args, mods) abort
     autocmd WinLeave <buffer> call s:WinLeave()
     autocmd WinEnter <buffer> call s:WinEnter()
   augroup END
-  nnoremap <buffer><silent> <Plug>(gina-blame-exit)
-        \ :<C-u>call <SID>exit_from_entire_blame()<CR>
+  nnoremap <buffer><silent> <Plug>(gina-blame-quit)
+        \ :<C-u>call <SID>exit_from_entire_blame('quit')<CR>
+  nnoremap <buffer><silent> <Plug>(gina-blame-close)
+        \ :<C-u>call <SID>exit_from_entire_blame('close')<CR>
   " Navi
   let bufname = gina#core#buffer#bufname(git, 'blame', {
         \ 'treeish': args.params.treeish,
@@ -127,8 +129,10 @@ function! s:init(args) abort
         \ :<C-u>call <SID>redraw_content()<CR>
   nnoremap <buffer><silent> <Plug>(gina-blame-C-L)
         \ :<C-u>call <SID>redraw_content()<CR>:execute "normal! \<C-L>"<CR>
-  nnoremap <buffer><silent> <Plug>(gina-blame-exit)
-        \ :<C-u>call <SID>exit_from_entire_blame()<CR>
+  nnoremap <buffer><silent> <Plug>(gina-blame-quit)
+        \ :<C-u>call <SID>exit_from_entire_blame('quit')<CR>
+  nnoremap <buffer><silent> <Plug>(gina-blame-close)
+        \ :<C-u>call <SID>exit_from_entire_blame('close')<CR>

   augroup gina_command_blame_internal
     autocmd! * <buffer>
@@ -189,10 +193,10 @@ function! s:get_blame_buffer_info() abort
   return {'current_name': bufname, 'alternative_name': alternate}
 endfunction

-function! s:exit_from_entire_blame() abort
+function! s:exit_from_entire_blame(method) abort
   let bufinfo = s:get_blame_buffer_info()
-  execute 'silent! bwipeout ' bufinfo.alternative_name
-  execute 'silent! bwipeout ' bufinfo.current_name
+  execute a:method bufnr(bufinfo.alternative_name)
+  execute a:method bufnr(bufinfo.current_name)
 endfunction

 function! s:redraw_content() abort

Tried out :hide, same results as :close.

I understand your worry about bwipeout but I simply cannot find a real solution which not using it.

EDIT

Forgot to say that :close does not closing the last window: for example hitting <Plug>(gina-blame-close) on the navi will close the content buffer, but navi stays open (even if it's silent!).

lambdalisue commented 7 years ago

I see. Give me time to consider (ping me on this weekend if i forgot to reply)

lambdalisue commented 7 years ago

One note. I'll ask you to write a document about that mapping after I dicide the way:+1:

bimlas commented 7 years ago

I think I found a good solution:

diff --git a/autoload/gina/command/blame.vim b/autoload/gina/command/blame.vim
index 3ee0101..bc5d72f 100644
--- a/autoload/gina/command/blame.vim
+++ b/autoload/gina/command/blame.vim
@@ -191,8 +191,15 @@ endfunction

 function! s:exit_from_entire_blame() abort
   let bufinfo = s:get_blame_buffer_info()
-  execute 'silent! bwipeout ' bufinfo.alternative_name
-  execute 'silent! bwipeout ' bufinfo.current_name
+  let original_buffer = gina#core#buffer#param(bufname('%'), 'path')
+  try
+    execute printf('%dclose', bufwinnr(bufinfo.alternative_name))
+  catch /^Vim\%((\a\+)\)\=:E444/
+    " E444: Cannot close last window may thrown but ignore that
+    " Vim.Buffer.Group should NOT close the last window so ignore
+    " this exception silently.
+  endtry
+  execute 'buffer ' original_buffer
 endfunction

 function! s:redraw_content() abort

It's closing the alternative window and opens the original buffer in the current window. It fails only if the original buffer is not exists, because in this case the blame buffer stays visible. Well, I cannot found a real world example for closing original buffer before exiting from blame.

lambdalisue commented 7 years ago

It's closing the alternative window and opens the original buffer in the current window.

I think

If there are more than two windows -> Close both window (navi and body of blame)
If there are less than or equal to two windows -> Close both window and open an empty window

is better while opening an original buffer may cost (e.g. a huge file with really complex syntax)

bimlas commented 7 years ago

Sorry for late reply, I had to relax: play video games, make some music, other stuff.

For me (and most of the users I think) the regular workflow of blame is:

Open a file in Vim
:Gina blame :
Walk the history of the file
Close blame
Edit the file itself

Opening an empty buffer instead of the original (where we opened up blame) would be annoying, because it's going to be closed immediately by user.

I tried to find a way to open up the most recent (original) buffer without success:

execute 'buffer ' original_buffer opens up the blame view
- :Gina blame :
- :ls, :bdelete original_buffer
- :map q <Plug>(gina-blame-exit)
- Press q
- The blame content window will be restored (because original_buffer stores its path instead of the original buffer)
:bprevious open up the previous buffer according to buffer number (instead of "historical" order)
The jumplist would be usefull, but I don't know how can I parse it to get the name of the most recent buffer

Maybe I complicating the things, but this is why this issue is long (regardless that it works):

since bufhidden=wipe wipes out anyway if the buffer gets hidden.

Well that's why I don't want to use bwipeout directly. I would make it possible to control the behavior by changing bufhidden options.

Can you give me an example of the effects of bufhidden? Is there a situation where :bwipeout can lead to issues?

lambdalisue commented 7 years ago

Now I feel that I should accept your opinion while I could not find any reason to keep mine. But before that, I would like to hear @itchyny 's opinion as well.

bimlas commented 7 years ago

I just found out a possible issue of bwipeout: the user wants to switch back to blame view from the original buffer while using only one window. But I think the current solution is OK until somebody starts a new issue about this.

itchyny commented 7 years ago

I read through the thread but I don't understand why :quit command (not mapping <Plug>(gina-blame-exit)) leaves the ~~window~~ buffer by default. Rather than providing custom mapping, cleaning up the windows would be nice for many people. If I am correct, we have to hack Buffer.Group of vital.vim, but it should be a nice default behaviour.

lambdalisue commented 7 years ago

@itchyny sorry my bad. It seems you are talking a bit different topic. I re-opened #89

lambdalisue commented 7 years ago

@bimlas Ok. I would like to try https://github.com/lambdalisue/gina.vim/pull/79#issuecomment-294352161 so could you update the branch with that patch?

bimlas commented 7 years ago

If you agree, I will add the documentation in the next commit.

lambdalisue commented 7 years ago

I tried. Generally I like the behavior but

Both window should be closed when there are tabs (namely, after :Gina blame --opener=tabnew :, user would like to close the tab)
It would be nice if :quit works like this behavior

I'm sure that 1. is not difficult but can you get it work with :quit rather than <Plug>(gina-blame-exit)? (It's OK if it is difficult)

bimlas commented 7 years ago

Well, :quit was easier to write: how can I determine that the user opened the file in a new tab, then used :Gina blame : or the tab is opened by :Gina blame --opener=tabnew :? The output of gina#core#meta#get_or_fail('params').opener is tabnew only for first time; going deeper in the blame is changing its original value.

bimlas commented 7 years ago

The last commit has an issue: if you want to close just the navi (via <C-W>q or :quit), then it's closing the entire blame, not just the left window. Is this OK? I cannot found a solution to close the entire blame with :quit, but close just the navi with <C-W>q, because those doing the same (invoking QuitPre autocommand).

bimlas commented 7 years ago

I had to amend the last commit (and force push): vint failed because unnecessary spaces.

lambdalisue commented 7 years ago

Sorry for take it long.

How about the following?

If winnr('$') returns more than two windows
1. If tabpagenr('$') returns more than one tab, close the current tab page
2. Otherwise close the navi window and open an original buffer on the body window
Otherwise close both windows

if you want to close just the navi

I don't think we have to care about that situation. :Gina blame opens two windows because of Vim's limitation. If Vim could handle columns and syntax more well, I would rather to open a single window for navi + body than opening two windows. So we should treat navi/body windows more like a single window.

bimlas commented 7 years ago

Forced push again because of vint.

I writing it down step-by-step:

:tabedit file
:Gina blame :
:quit
The user should see the original buffer again in the second tab

:edit file
:Gina blame --opener=tabnew :
:quit
The user should see the the firsr tab, the second (opened by Gina) is closed

tabpagenr('$') returns 2 in both, but it should be closed only in second case, but I cannot detect it because of https://github.com/lambdalisue/gina.vim/pull/79#issuecomment-298650914.

<Plug>(gina-blame-exit) closing the blame view and restores the original buffer without closing the tab. :quit closing the blame view and the tab (or exiting Vim if there is only one tab).

bimlas commented 7 years ago

I'm thinking on a solution: store the "user params" of the actions. For example, it would store opener: tabnew when the command is invoked (Gina blame --opener=tabnew :) and it keeps it for the whole blaming session. It would be visible on :quit (regardless that the user is in the initial view or viewing an older commit), so the function could decide what to do: restore the original buffer (there is no --opener) or close the window/tab (with --opener).

It would be useful for other commands too, for example

:Gina log --all
Doing Git stuff outside of Vim
Press <C-L> in the log window - it would invoke log with "user params" (`--all`)

bimlas commented 7 years ago

It would be consistent to add an action for exit, but as I see only local functions are used in autoload/gina/action/blame.vim. Should I extract the exit function to autoload/gina/command/blame/exit.vim and use it in command and action script too?

lambdalisue commented 7 years ago

Sorry for takin time. I'm going to read your comment now.

lambdalisue commented 7 years ago

but it should be closed only in second case

This is different from what I think. I think Gina should always close both window. The exception is when there are only two window is opened in a single tabpage.

lambdalisue commented 7 years ago

It would be consistent to add an action for exit

Well as long as :q works as expect. I think that is enough and no action is necessary

bimlas commented 7 years ago

As the commit message says my solution would be:

If `--opener` exists (!= `edit`), then close both window, otherwise close
alternative and switch to original buffer in current window.

- Open a blame view (regardless of `--opener`)
- Press `<Plug>(gina-blame-exit)`
- Works as expected

- Open a blame view with `--opener`
- Go deeper in the history (press `<CR>` in navi)
- Press `<Plug>(gina-blame-exit)`
- Blame view is closing, but the window isn't

The second case is because `--opener` exists only in first "view" of blame,
going deeper in the history will modify it to `--opener=edit`.

Your solution is

think Gina should always close both window. The exception is when there are only two window is opened in a single tabpage.

Let me explain in examples:

Opening a Vim function in a buffer (window 1)
:split edit the help of the function to write it (window 2)
I want to blame the function so running :Gina blame : in window 1
Reviewing history, I finished blaming and I want to get back the function in window 1

In your solution window 1 would be closed, while my solution keeps the window open and restoring original buffer (the function), because of --opener=edit.

Another example:

I'm a Vim newbie who preferring tabs over buffers so I am opening a new tab for every file
I want to blame the file in tab 2 (with a single window)
Finished blaming, I want to get back the original buffer in tab 2

In your solution the tab would be closed, because there are only 2 windows exists in it, while mine would keep it opened and restoring the original buffer (because of --opener=edit).

I like to open blame in a new split so I am using --opener=vsplit
Finished blaming, and I want to blame windows get closed

It works with both solution.

lambdalisue commented 7 years ago

I feel your solution is a bit less logical while it changes the behavior by --opener option. I think the following two case should close windows equally.

:Gina blame --opener=tabnew :README.md
:tabnew | Gina blame :README.md

While :Gina blame --opener={opener} :{filename} opens a buffer equal to :{opener} {filename}, the behavior for closing should follow Vim's flavor.

Let me explain in examples:

Opening a Vim function in a buffer (window 1)

:split edit the help of the function to write it (window 2)

I want to blame the function so running :Gina blame : in window 1

Reviewing history, I finished blaming and I want to get back the function in window 1

In your solution window 1 would be closed, while my solution keeps the window open and restoring original buffer (the function), because of --opener=edit.

If you want to get back, the correct command is not :q but <C-^> in Vim's normal buffer. Think the situation below

Open README.md by :e README.md
Open LICENSE by :e LICENSE
What kind of behavior do you expect here for :q? (Or how do you get back? I'm sure that you don't use :q here)

While <Plug>(gina-blame-quit) is a mapping for quit, I really want to close the window instead of get back.

Another example:

I'm a Vim newbie who preferring tabs over buffers so I am opening a new tab for every file

I want to blame the file in tab 2 (with a single window)

Finished blaming, I want to get back the original buffer in tab 2

In your solution the tab would be closed, because there are only 2 windows exists in it, while mine would keep it opened and restoring the original buffer (because of --opener=edit).

I like to open blame in a new split so I am using --opener=vsplit

Finished blaming, and I want to blame windows get closed

It works with both solution.

As I said, if you would like to get back, <Plug>(gina-blame-quit) is not for that. Like

Open README.md by :e README.md
Open LICENSE by :tabnew LICENSE
Open foo by :e foo
What kind of behavior do you expect here for :q? (Or how do you get back to LICENSE?)

lambdalisue commented 7 years ago

https://github.com/lambdalisue/gina.vim/commit/da85b1953833033df358da43d88a0d894608d8ce

lambdalisue / vim-gina

Add mapping to exit entire blame (#78) #79