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

per-instance max-width for popup instances #29

Closed
wants to merge 5 commits into from
Closed

per-instance max-width for popup instances #29

wants to merge 5 commits into from

Conversation

toumorokoshi
Copy link

Hi there,

I had a similar request for popup.el as #28, and I thought I'd contribute the per-instance based max-width, in a separate pull request as indicated. I also thought @laynor's addition to choosing the maximum width as ratio or character based value was cool so I added that in as well.

@@ -34,6 +34,14 @@

;;; Utilities


(defun calculate-max-width (width)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add popup- prefix. Also, please remove the trailing whitespace in the next line.

@tkf
Copy link
Contributor

tkf commented Jan 13, 2013

Other than the minor points I mentioned on the source code:

  • Why not add max-width also to popup-tip and popup-menu*?
  • This pull request also breaks "multiple of 10" restriction. If we remove this restriction in Maximum popup menu width option #28, we can pull this request as-is.

@toumorokoshi
Copy link
Author

Thanks for the tips! I'm not super knowledgeable about the elisp world so it's helpful to get comments.

I've added a couple commits that add max-width to popup-tip and popup-menu* as well as fixing those points made in the comments.

I made a separate commit for the rounding depending on whether you want the "multiple of 10" restriction or not. I thought it would make sense to enforce that when choosing a width based off of the window, but not for specific character lengths, so I left the latter case alone.

@m2ym
Copy link
Contributor

m2ym commented Jan 21, 2013

Great work but I'm skeptical about the ratio option of max-width. Is it really useful rather than just specifying max-width like :max-width (* 0.3 (window-body-widht)) at the client side? Is it sure there is no corner case?

By the way, I've added few comments on your commits. Please take a look.

@toumorokoshi
Copy link
Author

Thanks for catching that typo, I have no idea how that worked before! I've committed a fix.

I suppose the scaling max-width is not necessary, people can just set it like you stated. I think it would be useful though. I mainly wanted this ability because I ran into issues with auto-complete and found that a large summary fills the whole screen with no space for tips:

Screenshot from 2013-01-21 21:42:55

I personally want to set the max-width to a value relative to the buffer size, so I can see the tooltip regardless of how wide the buffer is:

Screenshot from 2013-01-12 23:06:51

I think most people want popups to scale relative to buffer width, not to a limited number of characters. Although I could be wrong.

Only corner case I can think of is the difference between 1 (1 character) and 1.0 (full width of buffer). I thought the distinction could be made clear by choosing an int or float.

@petrux
Copy link

petrux commented Nov 28, 2013

I agree with @toumorokoshi: it would be good to have popups that scale with the buffer width. I'm not that good in elisp (i.e. I just know it exists :-P) but I'd be glad to contribute in whatever way I can.

@toumorokoshi
Copy link
Author

@tkf @m2ym can we get this merged in? If the max-width being a proportion is a problem I can change it, I see the advantage to just having users set it explicitly.

syohex added a commit that referenced this pull request Jan 23, 2014
Patch of pull request #29 cannot be merged now. I fix it for
current popup.el.
@syohex
Copy link
Contributor

syohex commented Jan 25, 2014

@toumorokoshi I have merged max-width feature. Please report us if you have problems.
And sorry for too late reply.

Thanks for good changes.

@syohex syohex closed this Jan 25, 2014
@toumorokoshi
Copy link
Author

@syohex Thanks so much for merging this! I appreciate it, especially making a new request to make the merge easier :)

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.

5 participants