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.

Sign up for GitHub

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

Jump to bottom

Allow to reuse open window when go to definition #893

Merged
merged 4 commits into from
Feb 29, 2020
Merged

Allow to reuse open window when go to definition #893

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a174039
Allow to reuse open window when go to definition
Feb 26, 2020
b19aa71
Add help text
Feb 26, 2020
4541b08
Merge branch 'develop' into use-open-window
Feb 27, 2020
b912ee2
Do not use <SID> without a necessity
Feb 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
82 changes: 53 additions & 29 deletions autoload/phpactor.vim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,23 +314,22 @@ endfunction
"""""""""""""""""""""""
" Utility functions
"""""""""""""""""""""""
function! s:isOpenInCurrentWindow(filePath)
return expand('%:p') == a:filePath
endfunction

function! phpactor#_switchToBufferOrEdit(filePath)
if expand('%:p') == a:filePath
" filePath is currently open
if <SID>isOpenInCurrentWindow(a:filePath)
Copy link
Collaborator

@dantleech dantleech Feb 26, 2020 •
edited

Choose a reason for hiding this comment

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

what is <SID> ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When you use a function that is defined as local to the script (function s:foo()), then using at the call as the prefix allows to avoid ambiguities especially when there exist a local function with the same name and both are run (non directly) from any external (as usually when using autoloaded functions) script. See :help SID.

From the technical point of view local functions are not fully protected from running from outside the script. If you run :function to see the list of defined functions then you may notice that local functions are also listed there. For example, at the moment I see a line <SNR>120_string_encode(str) abort (probably from vim-unimpaired)
and I can use it by echo <SNR>120_string_encode('zxc').

Copy link
Collaborator

@dantleech dantleech Feb 29, 2020 •
edited

Choose a reason for hiding this comment

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

So if I understand correctly, we use the s: scope to make this a "private" function. I don't understand why we can't call s:isOpenCurrentWindow instead of <SID>isOpenCurrentWindow though?

make it local to the script. But when a mapping is executed from outside of the script, it doesn't know in which script the function was defined. To avoid this problem, use <SID> instead of "s:". The same translation is done

This doesn't sound like it's the case here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we may consider them "private" function (local to the script) remembering that we can still call it prefixed with a script number as above.

I made basic test both on vim and neovim that suggested that using is unnecessary when a function is not called from a mapping. Indeed, the help text stands only about mapping, but it could sound more clear like there.

Thanks for the question.

return v:false
endif

let bufferNumber = bufnr(a:filePath . '$')

if (bufferNumber == -1)
exec ":edit " . a:filePath
return v:true
endif

exec ":buffer " . bufferNumber
let command = (bufferNumber == -1)
\ ? ":edit " . a:filePath
\ : ":buffer " . bufferNumber

return v:true
exec command
endfunction

function! phpactor#_offset()
Expand Down Expand Up @@ -434,9 +433,9 @@ function! phpactor#rpc(action, arguments)
let result = system(cmd, json_encode(request))

if (v:shell_error == 0)
try
try
let response = json_decode(result)
catch
catch
throw "Could not parse response from Phpactor: " . v:exception
endtry

Expand Down Expand Up @@ -510,27 +509,17 @@ function! phpactor#_rpc_dispatch(actionName, parameters)
if a:actionName == "open_file"
let changedFileOrWindow = v:true

if a:parameters['target'] == 'focused_window'
let changedFileOrWindow = phpactor#_switchToBufferOrEdit(a:parameters['path'])

if a:parameters['force_reload'] == v:true
exec "e!"
endif
endif

if a:parameters['target'] == 'vsplit'
exec ":vsplit " . a:parameters['path']
endif

if a:parameters['target'] == 'hsplit'
exec ":split " . a:parameters['path']
endif
call <SID>openFileInSelectedTarget(
\ a:parameters["path"],
\ a:parameters["target"],
\ get(a:parameters, "use_open_window", g:phpactorUseOpenWindows),
\ a:parameters["force_reload"]
\ )

if a:parameters['target'] == 'new_tab'
exec ":tabnew " . a:parameters['path']
if a:parameters["target"] == 'focused_window'
let changedFileOrWindow = !<SID>isOpenInCurrentWindow(a:parameters["path"])
endif


if (a:parameters['offset'])
let keepjumps = changedFileOrWindow ? 'keepjumps' : ''

Expand Down Expand Up @@ -653,6 +642,41 @@ function! phpactor#_rpc_dispatch(actionName, parameters)
throw "Do not know how to handle action '" . a:actionName . "'"
endfunction

function! s:openFileInSelectedTarget(filePath, target, useOpenWindow, forceReload)
let bufferNumber = bufnr(a:filePath . "$")
if v:true == a:useOpenWindow && -1 != bufferNumber
let firstWindowId = get(win_findbuf(bufferNumber), 0, v:null)

if v:null != firstWindowId
call win_gotoid(firstWindowId)
return
endif
endif

if a:target == 'focused_window'
call phpactor#_switchToBufferOrEdit(a:filePath)
if v:true == a:forceReload
exec "e!"
endif
return
endif

if a:target == 'vsplit'
exec ":vsplit " . a:filePath
return
endif

if a:target == 'hsplit'
exec ":split " . a:filePath
return
endif

if a:target == 'new_tab'
exec ":tabnew " . a:filePath
return
endif
endfunction

function! phpactor#_rpc_dispatch_input_handler(Next, parameters, parameterName, result)
let a:parameters[a:parameterName] = a:result

Expand Down
5 changes: 5 additions & 0 deletions doc/phpactor.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,11 @@ use the VIM quick-fix list.
Function to use when presenting a user with a choice of options. The default
is to use the VIM inputlist.

*g:phpactorUseOpenWindows*
When jump to the line of a file displayed in any existing window reuse this
window to avoid have more than one view of the same file. The default is
false.

==============================================================================
COMPLETION *phpactor-completion*

Expand Down
69 changes: 31 additions & 38 deletions ftplugin/php.vim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,48 +9,41 @@ let g:phpactorInitialCwd = getcwd()
let g:phpactorCompleteLabelTruncateLength=50
let g:_phpactorCompletionMeta = {}

if !exists('g:phpactorPhpBin')
""
" Path to the PHP binary used by Phpactor
let g:phpactorPhpBin = 'php'
endif

if !exists('g:phpactorBranch')
""
" The Phpactor branch to use when calling @command(PhpactorUpdate)
let g:phpactorBranch = 'master'
endif

if !exists('g:phpactorOmniAutoClassImport')
""
" Automatically import classes when using VIM native omni-completion
let g:phpactorOmniAutoClassImport = v:true
endif

if !exists('g:phpactorCompletionIgnoreCase')
""
" Ignore case when suggestion completion results
let g:phpactorCompletionIgnoreCase = 1
endif

if !exists('g:phpactorQuickfixStrategy')
""
" Function to use when populating a list of code references. The default
" is to use the VIM quick-fix list.
let g:phpactorQuickfixStrategy = 'phpactor#quickfix#vim'
endif

if !exists('g:phpactorInputListStrategy')
""
" Function to use when presenting a user with a choice of options. The default
" is to use the VIM inputlist.
let g:phpactorInputListStrategy = 'phpactor#input#list#inputlist'
endif
""
" Path to the PHP binary used by Phpactor
let g:phpactorPhpBin = get(g:, 'phpactorPhpBin', 'php')

""
" The Phpactor branch to use when calling @command(PhpactorUpdate)
let g:phpactorBranch = get(g:, 'phpactorBranch', 'master')

""
" Automatically import classes when using VIM native omni-completion
let g:phpactorOmniAutoClassImport = get(g:, 'phpactorOmniAutoClassImport', v:true)

""
" Ignore case when suggestion completion results
let g:phpactorCompletionIgnoreCase = get(g:, 'phpactorCompletionIgnoreCase', 1)

""
" Function to use when populating a list of code references. The default
" is to use the VIM quick-fix list.
let g:phpactorQuickfixStrategy = get(g:, 'phpactorQuickfixStrategy', 'phpactor#quickfix#vim')

""
" Function to use when presenting a user with a choice of options. The default
" is to use the VIM inputlist.
let g:phpactorInputListStrategy = get(g:, 'phpactorInputListStrategy', 'phpactor#input#list#inputlist')

""
" When jump to the line of a file displayed in any existing window
" reuse this window to avoid have more than one view of the same file.
" The default is false.
Copy link
Collaborator

@dantleech dantleech Feb 26, 2020 •
edited

Choose a reason for hiding this comment

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

When jumping to a file location: if the target file open in a window, switch to that window instead of switching buffers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I cannot judge what version is more proper. Feel free to choose the better in your opinion.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, if what I said above is technically correct, then it could be better 👍

let g:phpactorUseOpenWindows = get(g:, 'phpactorUseOpenWindows', v:false)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This syntax looks potentially better, but can we use the same as the above for consistency in this PR?

We are using vimdoc to generate the docuemntation. Can you add a description of this option as with the other options?

Copy link
Collaborator

Choose a reason for hiding this comment

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

(bonus if you can install vimdoc and update the help file by running vimdoc . in the project root :) )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Vimdoc seems to be unavailable from pip repo but I have installed it manually without any problem. I don't know why vimdoc make this removal

-NOTE: This help is auto-generated from the VimScript using
-https://github.com/google/vimdoc. See
-https://phpactor.github.io/phpactor/developing.html#vim-help
-

and I have to restore it manually.

Please review all these changes for English - also function and variable names ;)

Rector for vim viml is needed ;)

Copy link
Collaborator

Choose a reason for hiding this comment

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

and I have to restore it manually.

I think I added it manually instead of adding it to the autoload/phpactor.vim source file ...


if g:phpactorOmniAutoClassImport == v:true
autocmd CompleteDone *.php call phpactor#_completeImportClass(v:completed_item)
endif


" vim: et ts=4 sw=4 fdm=marker