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

Add recipe for frontside-windowing #7569

Closed

Conversation

cowboyd
Copy link
Contributor

@cowboyd cowboyd commented May 13, 2021

Brief summary of what the package does

When using compilation buffers or other commands that split windows vertically or horizontally, the way in which Emacs splits windows can vary greatly depending on the frame size. This adds some consistency on how the windows split by making a preference to splitting the window evenly in half, and keeping it to two splits.

This is the technique we've been using for years, and so wanted to make it generally available as we try and break out our monolithic configuration into a bunch of loosely coupled packages.

Direct link to the package repository

https://github.com/thefrontside/frontmacs/tree/release/packages/windowing

Your association with the package

maintainer

Relevant communications with the upstream package maintainer

None needed

Checklist

When using compilation buffers or other commands that split windows
vertically or horizonally, this adds some consistency by making a
preference to splitting the window evenly in half, and keeping it to
two splits.
@riscy
Copy link
Member

riscy commented May 16, 2021

Thanks for this -- very quick first pass:

  • frontside-windowing.el#L1: lexical-binding must be on the end of the first line
  • frontside-windowing.el#L13: Use empty lines instead of ;; on a line by itself within your Commentary
  • frontside-windowing.el#L45: Don't set global bindings; tell users how in your ;;; Commentary because many users could be using bindings other than C-x 2/3 to split their windows. Alternatively, this package seems to be a good match for a global minor mode with its own (customizable) keybindings that can be easily turned off again
  • This package seems to do things outside of its stated scope, e.g. it disables the menu bar, inhibits the startup screen, removes the scrollbar -- all of this extra stuff should be pared off
  • Add license boilerplate or an SPDX-License-Identifier to frontside-windowing.el (in case someone only receives the file and not your repo's LICENSE file)
  • Have you had a chance to compare this to other packages in MELPA with similar functions? One popular example is https://github.com/roman/golden-ratio.el

cowboyd added a commit to thefrontside/frontmacs that referenced this pull request May 20, 2021
@cowboyd
Copy link
Contributor Author

cowboyd commented May 20, 2021

@riscy I've taken the opportunity to refactor this to use a globalized minor-mode per your suggestion. I didn't know that it was a thing, thanks for the tip! I've also taken care of the other pieces of feedback.

I have search for something similar on MELPA. The goal here is to be as dead simple as possible. Splitting sensibly means always split in half, and to the right, nothing fancier. This has worked for us for years.

@riscy
Copy link
Member

riscy commented May 23, 2021

Thanks, that looks like a good way to isolate the keybindings. I think it would be beneficial to actually defvar (or defcustom if you're feeling brave) that keybinding, because creating a variable gives users a direct and simple way to tailor the minor mode to whatever keybindings they like. e.g. (defvar frontside-windowing-mode-map (let ((keymap ...)). And then within your define-minor-mode you can specify :keymap frontside-windowing-mode-map.

I think you may not need a globalized minor mode (since you don't require a TURN-ON function that makes fancy checks), and you can instead specify :global t within your define-minor-mode.

While I'm here, a couple minor checkdocs popped up this time around:

Warning (emacs): 
frontside-windowing.el:52: First sentence should end with punctuation
Warning (emacs): 
frontside-windowing.el:52: First line should be capitalized

@riscy
Copy link
Member

riscy commented May 29, 2021

I'll mark this awaiting upstream for now, but feel free to reach out any time.

@riscy riscy added the awaiting-upstream Awaiting action from an upstream maintainer label May 29, 2021
@cowboyd
Copy link
Contributor Author

cowboyd commented May 30, 2021

@riscy I tried using :global true, but toggling it off did not seem to actually work. I wasn't exactly sure, but it looked like this was because it created a custom variable that was persistent.

@riscy riscy removed the awaiting-upstream Awaiting action from an upstream maintainer label May 30, 2021
@riscy
Copy link
Member

riscy commented May 31, 2021

Hmm, can you paste the code you tried using?

@riscy
Copy link
Member

riscy commented Jun 19, 2021

Friendly ping. :)

@cowboyd
Copy link
Contributor Author

cowboyd commented Jun 28, 2021

I haven't been able to dedicate any cycles to this, but will try when I get the opportunity.

@riscy riscy added the awaiting-upstream Awaiting action from an upstream maintainer label Jul 3, 2021
@riscy
Copy link
Member

riscy commented Jul 3, 2021

I'll mark this awaiting upstream for now, but feel free to reach out.

@tarsius
Copy link
Member

tarsius commented May 13, 2022

I am closing this pull-request because it is over a year old now. That doesn't mean that we won't add this to Melpa, just that you have to ask us to reopen it once you are ready.

@tarsius tarsius closed this May 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-upstream Awaiting action from an upstream maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants