lbonn / i3-quickterm

A small drop-down terminal for i3 and sway
MIT License
128 stars 13 forks source link

remember shell resizes between toggles #12

Open laur89 opened 4 years ago

laur89 commented 4 years ago

Fixes #11

lbonn commented 4 years ago

Thanks, that's an interesting feature.

I do have a few concerns though:

Couldn't the same effect be achieved by storing data in some temporary file and keeping a "daemon-less" structure? Also better if the feature can be disabled via a switch.

laur89 commented 4 years ago

it does not work when using the menu (no shell given as argument)

Current features haven't been broken, including the menu option.

the commit also refactors/move things around, it would be best if these modifications could be split

Agreed, got carried away.

do we really need a daemon just to provide this feature?

We don't, but imho it's a cleaner option than IO via storage.

Some people won't need it but would have to add this daemon running in the background, making it a bit more brittle for relatively few benefit.

A bit more brittle, but don't see it becoming an issue. Making the feature opt-in would be an option, but would complicate the code.

Regardless how this PR goes, note you'd still want the 3rd point from the commit message - that's an unrelated bug this fixes.

lbonn commented 4 years ago

it does not work when using the menu (no shell given as argument)

Current features haven't been broken, including the menu option.

The menu still works but the window size is not conserved when using it. Yet, it still fails early if the daemon is not running.

do we really need a daemon just to provide this feature?

We don't, but imho it's a cleaner option than IO via storage.

The daemon mode is a bit complex right now. Moreover, as the original code already deals with different code running in different processes. But, I would still prefer if using a daemon is optional and not having one at all seems even better on an usability perspective.

Regardless how this PR goes, note you'd still want the 3rd point from the commit message - that's an unrelated bug this fixes.

Yes, could you please split that up in another PR or at least a separate commit, before the new feature?