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

Improve some godoc's popup window #2447

Closed
wants to merge 0 commits into from
Closed

Improve some godoc's popup window #2447

wants to merge 0 commits into from

Conversation

skanehira
Copy link
Contributor

I improve some :)

  • Only close godoc's popup window
  • Can to be able scroll popup window
  • Display a window in the center of the scree

asciicast

@bhcleek
Copy link
Collaborator

bhcleek commented Aug 9, 2019

Thank you for the contribution. When the current tab is split into two windows, though, I suspect that this won't work quite the way you might expect. A few things to consider:

  • a user has two windows open and tries to show godoc in each.
  • the window in which the popup will be shown is 10 lines tall or less
  • the window in which the popup will be shown is 10 columns wide or less

@skanehira
Copy link
Contributor Author

skanehira commented Aug 9, 2019

@bhcleek

a user has two windows open and tries to show godoc in each.

I think if user want to open more one godoc, use buffer is more better.
also, I think that there is no problem because the user can choose whether or not to use the popup window.

the window in which the popup will be shown is 10 lines tall or less
the window in which the popup will be shown is 10 columns wide or less

How about this fixing?

          \ 'maxheight': &lines,
          \ 'maxwidth': &columns

@bhcleek
Copy link
Collaborator

bhcleek commented Aug 9, 2019

I think if user want to open more one godoc, use buffer is more better.

The option is global, though, and doesn't take into account how many windows are open, nor does it take into account how many tabs are open, it simpy closes the last opened godoc popup window. I think this can be handled better by making s:last_popup_window an object whose keys are the windows that contain the popup windows and the values are the popup window ids.

 \ 'maxheight': &lines,
 \ 'maxwidth': &columns

I'll think about ☝️ and get back to you

@skanehira
Copy link
Contributor Author

The option is global, though, and doesn't take into account how many windows are open, nor does it take into account how many tabs are open, it simpy closes the last opened godoc popup window. I think this can be handled better by making s:last_popup_window an object whose keys are the windows that contain the popup windows and the values are the popup window ids.

It's easy to display multiple popup windows, but how do you think it should be displayed and controlled?

I think the following controls are needed.

  • requires key binding to focus between multiple popup windows
  • since user cannot do any thing the current window while displaying popup windows, user must close all popup windows or disable popup window's filter function when want to do something.
  • when disable popup window's filter after, if user want to scroll godoc in popup window, must run some Ex command to focus to popup window.

In this way, the operation becomes complicated, so i think that usability will be reduced.
If you have a good idea, please let me know

@bhcleek
Copy link
Collaborator

bhcleek commented Aug 10, 2019

I'll play with this and get back to you.

@skanehira
Copy link
Contributor Author

@bhcleek Hi, how about this PR?

@bhcleek
Copy link
Collaborator

bhcleek commented Aug 15, 2019

@skanehira I'll get to this soon. I have a couple of things in front of it that I need to get done first. I'll update here as soon as I've had a chance to review more thoroughly.

@skanehira
Copy link
Contributor Author

@bhcleek I got it :)

@bhcleek bhcleek added this to the vim-go 1.22 milestone Aug 31, 2019
Copy link
Collaborator

@bhcleek bhcleek left a comment

Choose a reason for hiding this comment

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

I finally had a chance to look at this in more detail. Thank you for taking the time to contribute and for being patient while it sat here without feedback.

I believe that popup_atcursor is preferable to popup_menu, because the doc window is not a menu and shouldn't behave like one. However, maybe it makes sense to use popup_create instead of popup_atcursor so that the syntax highlighting and the filetype can be set for the buffer. Can you tell me more about why you prefer the doc window to be in the center of the screen instead of near the cursor?

Using popup_atcursor would raise questions about whether there should be a filter or not, and if so, what it should be.

Have you encountered some problems with the current implementation that motivated you to set maxheight and maxwidth?

doc/vim-go.txt Outdated

Use this option to dislpay godoc in popup window. By default is disable.
Copy link
Collaborator

Choose a reason for hiding this comment

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

dislpay -> display

@bhcleek
Copy link
Collaborator

bhcleek commented Nov 28, 2019

@skanehira should I close this?

@skanehira
Copy link
Contributor Author

@bhcleek I'm sorry for being late.

Can you tell me more about why you prefer the doc window to be in the center of the screen instead of near the cursor?

If window popup at the screen center, window can use more screen space for long doc.
And, use popup_menu can scroll long doc.

Have you encountered some problems with the current implementation that motivated you to set maxheight and maxwidth?

I want to watch long doc using more screen space, so I used maxheight and max width.

@bhcleek
Copy link
Collaborator

bhcleek commented Nov 29, 2019

popup_menu sets more options than are appropriate for this. Can you switch it to popup_atcursor instead? You should still be able to get the scrolling behavor you desire.

@skanehira
Copy link
Contributor Author

@bhcleek Ok, I'll use popup_atcursor 👍

@bhcleek
Copy link
Collaborator

bhcleek commented Jan 12, 2020

@skanehira I'm wondering whether you plan to finish this up? If not, that's ok; let me know so I can decide what to do with this PR.

@bhcleek bhcleek removed this from the vim-go 1.22 milestone Jan 12, 2020
@skanehira skanehira closed this Jan 13, 2020
@skanehira
Copy link
Contributor Author

@bhcleek Sorry, I'm so late..
after much consideration,I thought that this PR was unnecessary.
Therefore, it will be closed.

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