agzam / spacehammer

Hammerspoon config inspired by Spacemacs
MIT License
567 stars 70 forks source link

Allow modal alert to select screens #95

Closed Grazfather closed 3 years ago

Grazfather commented 3 years ago

This extends #75 to allow a user to declare their own get-alert-screens to overwrite the default.

This allows a user to write their own function to use whatever logic they want, but also provides three helpers: main, primary (whichever the focused window is on), and all.

agzam commented 3 years ago

Do we really need five different [global] functions for this? Perhaps one parameterized would suffice?

Grazfather commented 3 years ago

I am happy to change the interface, but how do you propose that would look like? Do you mean expose some config such as

(global get-alert-screens-opt :all)

and having get-alert-screens call the appropriate local function? Only real issue I see there is that you then lose the ability to provide your own function, but maybe get-alert-screens could take a keyword OR a function.

agzam commented 3 years ago

The main question is, what's the goal here?

The initial purpose of core/alert was to have a simple shortcut to hs.alert (without altering much of its functionality), and that function probably shouldn't have been global to begin with.

But it is what it is, and now, If we need to extend it, adding more global shortcuts perhaps is not the wisest option here.

If the goal is to add the optional screen parameter, that's pretty straightforward. But it seems you also would like to make it, so it displays alerts on every available screen. Which to be honest, I don't know how would scale - since I have no way of testing this.

I guess the only way to find out is to make it and hope that someone, say with six displays won't complain one day that everything is too slow.

Grazfather commented 3 years ago

copying my comment from #75

I would like this, in fact, I started writing my own.

In my case my laptop monitor is considered the main, but I mostly don't look at it. I'd like to see the modal no matter which screen I happen to be looking at.

This could be configurable, maybe: something in config.fnl that lets you define a function that generates a list of screens (fn [] [(hs.screen.primaryScreen)]) could be the default.

I still want something that shows on both screens, and I just thought the configurable part would be nice.

I would not suspect that alerting on 6 screens is much worse than on one. I just use two and it hasn't been an issue.

Maybe just a :alert-screens part of config could work, and have the example/default config have a getter that returns a list with just the primary screen. Two issues:

  1. People with old configs won't have this in their config, so we need to handle when the config is empty.
  2. Since the screens would be calculated at the time when the config is loaded we don't properly handle things such as unplugging or plugging in new screens.
agzam commented 3 years ago

People with old configs won't have this in their config

Yeah, we kinda screwed it up a bit. We need to revamp our custom config story.

Grazfather commented 3 years ago

Yes, maybe that can be done for a 1.0 :)

Now for this PR in particular, I can revamp it easily. core is already provided config. I propose that if config contains an optional :alert-screens-fn then that function will be used to fetch which screens to send alerts to, otherwise use my function in this PR, but with get-alert-screens-primary as the getter.

We also don't need to expose any of the get-alert* stuff this way. I can also modify the example config.fnl to add a blurb about setting that key.

Grazfather commented 3 years ago

I messed this branch up, since it was based on master, so this merge was a no-op. I think I lost the changes, too, but hopefully #101 obviates it.