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

Continous saving of current tabs and tab-zooms #742

Merged
merged 17 commits into from
Nov 2, 2023

Conversation

ldrahnik
Copy link
Contributor

@ldrahnik ldrahnik commented Oct 29, 2023

Fixes #741

@ldrahnik ldrahnik marked this pull request as ready for review October 30, 2023 07:42
Copy link
Collaborator

@jeremypw jeremypw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is mostly working - thanks! A couple of issues:

  • Changing the default font with <Control>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.

  • 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.

@ldrahnik ldrahnik force-pushed the continuous-remember-tabs branch from 343f4da to 24ce4cc Compare October 30, 2023 14:24
@ldrahnik
Copy link
Contributor Author

ldrahnik commented Oct 30, 2023

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.

8b126dd

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.

24ce4cc

src/MainWindow.vala Outdated Show resolved Hide resolved
src/MainWindow.vala Outdated Show resolved Hide resolved
Copy link
Collaborator

@jeremypw jeremypw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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)

I have made a couple of other comments inline.

@ldrahnik ldrahnik force-pushed the continuous-remember-tabs branch from 552e4eb to c963ac5 Compare October 30, 2023 18:24
@ldrahnik ldrahnik force-pushed the continuous-remember-tabs branch from c963ac5 to 0cbd59d Compare October 30, 2023 18:28
@ldrahnik
Copy link
Contributor Author

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)

0a0d06c

@ldrahnik
Copy link
Contributor Author

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

2574910

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
Copy link
Contributor Author

ldrahnik commented Oct 30, 2023

db5843a

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.

@ldrahnik ldrahnik force-pushed the continuous-remember-tabs branch from 9a9d24a to db5843a Compare October 30, 2023 22:41
@jeremypw
Copy link
Collaborator

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

src/MainWindow.vala Outdated Show resolved Hide resolved
@ldrahnik
Copy link
Contributor Author

ldrahnik commented Oct 31, 2023

@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
Copy link
Collaborator

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

@jeremypw
Copy link
Collaborator

jeremypw commented Nov 1, 2023

@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 ldrahnik force-pushed the continuous-remember-tabs branch from 6f679aa to 2e059a2 Compare November 1, 2023 13:26
@ldrahnik
Copy link
Contributor Author

ldrahnik commented Nov 1, 2023

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)

2e059a2

Copy link
Collaborator

@jeremypw jeremypw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is good to go now - thanks for your contribution!

@jeremypw
Copy link
Collaborator

jeremypw commented Nov 1, 2023

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

Copy link
Collaborator

@jeremypw jeremypw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, missed the fact that one of the unit tests now fails (CI was failing anyway in other PRs for an unrelated reason). The error given is

▶ 2/3 ERROR:../src/tests/Application.vala:155:__lambda53_: assertion failed (n_tabs == 1): (2 == 1) ERROR 

Not immediately obvious why this PR should affect the number of tabs - I'll investigate.

@jeremypw
Copy link
Collaborator

jeremypw commented Nov 1, 2023

@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
Copy link
Contributor Author

ldrahnik commented Nov 1, 2023

@jeremypw Merged.

@ldrahnik ldrahnik requested a review from jeremypw November 2, 2023 07:10
@jeremypw jeremypw merged commit b2b9c95 into elementary:master Nov 2, 2023
4 checks passed
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.

Tabs and tab-zooms not saved to settings continuously
2 participants