elementary / terminal

Terminal emulator designed for elementary OS
https://elementary.io
GNU Lesser General Public License v3.0
399 stars 96 forks source link

Continous saving of current tabs and tab-zooms #742

Closed ldrahnik closed 8 months ago

ldrahnik commented 8 months ago

Fixes #741

ldrahnik commented 8 months ago

Changing the default font with scroll does not change settings because at the moment the Terminal Widget changes the font directly. You need to change the scroll handler to activate the relevant window action instead.

https://github.com/elementary/terminal/pull/742/commits/8b126dd31bf1c43c8461938b48245b3967c38e53

I suggest you split the save_opened_terminals function up so that you can change the zooms and uris settings independently to save unnecessary work being done.

https://github.com/elementary/terminal/pull/742/commits/24ce4cc781da0c79e634cc0ff484c14c62d95942

ldrahnik commented 8 months ago

Adding another signal to the terminal widget is not the best solution - you can just activate the appropriate action in the scroll handler instead of calling increment_size and decrement_size. This can be done with code like window.get_simple_action (MainWindow.ACTION_ZOOM_FONT_IN).activate (null); (other examples of this pattern are already used in the code)

https://github.com/elementary/terminal/pull/742/commits/0a0d06c9c9f4263deddfbefa450788166df8331e

ldrahnik commented 8 months ago

You seem to have repeated a lot of code in save_open_terminals_with_zooms? Can you just call the split functions?

https://github.com/elementary/terminal/pull/742/commits/2574910eb4aea85e98ff250682aac87b780b85b5

Maybe would be a better solution to make again one function but with bool value save_zooms to avoid duplicated cycles though opened terminals.

I think I processed all notes. Thank you for reviewing.

ldrahnik commented 8 months ago

https://github.com/elementary/terminal/pull/742/commits/db5843a0f0b84f6eda90e4a710dc0a000459d116

Handle the situation when is opened first terminal, then second one, second one is closed and then is laptop rebooted. Without this code would be saved tabs and tab-zooms from second terminal but should be from first one.

jeremypw commented 8 months ago

@ldrahnik I am fine with you merging the saving functions back into one with an extra parameter.

ldrahnik commented 8 months ago

@jeremypw Refactored to one function but with 2 bool parameters because I realized I do not know how to do that with only 1 - 1. state save only tabs, 2. state save only zooms, 3. state save both

jeremypw commented 8 months ago

It could be done with an enum as a parameter but probably not worth it in this case.

jeremypw commented 8 months ago

@ldrahnik One small thing I noticed - save_opened_terminals () get called before current_terminal is set leading to the critical warning gdouble vte_terminal_get_font_scale)VteTerminal*): assertion VTE_IS_TERMINAL(terminal) failed (MainWindow lin 1361)

Other than that it seems to work well now!

ldrahnik commented 8 months ago

One small thing I noticed - save_opened_terminals () get called before current_terminal is set leading to the critical warning gdouble vte_terminal_get_font_scale)VteTerminal*): assertion VTE_IS_TERMINAL(terminal) failed (MainWindow lin 1361)

https://github.com/elementary/terminal/pull/742/commits/2e059a23f0201c09caa4a23617d9488c4438e7bc

jeremypw commented 8 months ago

Need to fix CI workflow (or override) before can be merged.

jeremypw commented 8 months ago

@ldrahnik I have pushed a PR to merge into this one that should fix the unit testing issue for now. The unit tests should probably be rewritten/extended to take continuous saving (or not) into account but that is complicated so can be left for another PR.

ldrahnik commented 8 months ago

@jeremypw Merged.