i3-quickterm icon indicating copy to clipboard operation
i3-quickterm copied to clipboard

remember shell resizes between toggles

Open laur89 opened this issue 4 years ago • 3 comments

  • make quickterm remember per-shell heigh ratio so our resizes would be persisted across toggles;
  • add [-d,--daemon] option to start a daemon process managing stateful data (per-shell ratios) and shell toggling;
  • in bring_up(), make sure container is first moved to scratchpad, and then resized/positioned - otherwise behaviour is erratic in multi-mon setups;

Fixes #11

laur89 avatar Oct 28 '19 20:10 laur89

Thanks, that's an interesting feature.

I do have a few concerns though:

  • it does not work when using the menu (no shell given as argument)
  • the commit also refactors/move things around, it would be best if these modifications could be split in smaller one-purpose commits with appropriate commit descriptions, to ease review
  • do we really need a daemon just to provide this feature? 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.

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.

lbonn avatar Oct 28 '19 23:10 lbonn

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.

laur89 avatar Oct 28 '19 23:10 laur89

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?

lbonn avatar Oct 29 '19 12:10 lbonn