Skip to content

Conversation

@muhitsarwar
Copy link

use g:bm_stack_mode = 1 to enable stack mode

ref: #157

Copy link
Owner

@MattesGroeger MattesGroeger left a comment

Choose a reason for hiding this comment

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

Hi @muhitsarwar,

thanks a lot for your contribution and congrats for getting your hands dirty with a vim plugin 💪 . I tried to get your changes to work but was unable to verify its functionality. Could you please describe step-by-step how I can successfully test it? It always showed the same order for me.

Apart from that I made some comments in the code and would also kindly ask you to document your new configuration variable in the Readme.md.

Thanks again and sorry it took me so long to respond 😔

call s:set('g:bookmark_location_list', 0 )
call s:set('g:bookmark_disable_ctrlp', 0 )
call s:set('g:bookmark_display_annotation', 0 )
call s:set('g:bm_stack_mode', 0 )
Copy link
Owner

Choose a reason for hiding this comment

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

The naming scheme is inconsistent here, it should be g:bookmark_stack_mode.

return keys(g:line_map[a:file])
endfunction

function! bm#location_list_stack_mode()
Copy link
Owner

Choose a reason for hiding this comment

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

There is a lot of duplication created due to this new function. It would be much nicer to have a sorting param passed to the existing function. The added benefit would be that we would not have two different function that use slightly different logic.


" Private {{{

function! bm#compare_bm(bm1, bm2)
Copy link
Owner

Choose a reason for hiding this comment

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

The function is comparing by sign_idx, maybe bm#compare_sign_idx would be a more descriptive name? In fact in order to make it consistent with the existing behaviour of bm#compare_lines we could just pass in the sign_idx.

endfunction

function! s:show_location()
if g:bm_stack_mode
Copy link
Owner

Choose a reason for hiding this comment

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

I like that you extracted this into it's own function as it gets more complicated now. If we could just parametrize bm#location_list() this whole function would become more readable.

Untested code:

function! s:show_location()
  lgetexpr bm#location_list(g:bm_stack_mode)
  if g:bookmark_location_list
    belowright lopen
  else
    belowright copen
  endif
endfunction

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