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

remember shell resizes between toggles #12

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

laur89
Copy link

@laur89 laur89 commented Oct 28, 2019

  • 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

@lbonn
Copy link
Owner

lbonn commented Oct 28, 2019

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.

@laur89
Copy link
Author

laur89 commented Oct 28, 2019

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.

- 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 lbonn#11
@laur89 laur89 force-pushed the feature/persist-resizes branch from bd220f0 to fd0228a Compare October 29, 2019 00:00
@lbonn
Copy link
Owner

lbonn commented Oct 29, 2019

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 lbonn force-pushed the master branch 2 times, most recently from 1bd9be6 to d94abd8 Compare January 16, 2022 18:47
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.

[FR] remember shell resizes between toggles
2 participants