Skip to content

Fix split terminal did not preverse font scale #1747 - #2046

Merged
Davidy22 merged 1 commit into
Guake:masterfrom
ChristianCelora:issue_1747
Apr 9, 2022
Merged

Fix split terminal did not preverse font scale #1747#2046
Davidy22 merged 1 commit into
Guake:masterfrom
ChristianCelora:issue_1747

Conversation

@ChristianCelora

@ChristianCelora ChristianCelora commented Feb 25, 2022

Copy link
Copy Markdown
Contributor

Following #1747, i saved the current font scale so when a terminal is split the font scale is not resized to default value.

mlouielu
mlouielu requested changes Feb 25, 2022
View reviewed changes

@mlouielu mlouielu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for your contribution, some points need to be polished:

  1. Commit message should write more clearly, for example: Fix split terminal should preserve font scale #1747
  2. Should this be update by font_scale? we may no need to add another attribute for this.

@mlouielu mlouielu changed the title fix issue #1747 Fix split terminal did not preverse font scale #1747 Feb 25, 2022
@ChristianCelora

ChristianCelora commented Feb 28, 2022

Copy link
Copy Markdown
Contributor Author

Thanks for the review. I am new to the open source world.

About the second point, i had found another solution, without creating a new attribute, by modifying the split() function and using the font_scale.

@ChristianCelora
ChristianCelora requested a review from mlouielu March 1, 2022 13:03
@mlouielu mlouielu added this to the 3.8.6 milestone Mar 1, 2022
@mlouielu

mlouielu commented Mar 1, 2022

Copy link
Copy Markdown
Collaborator

I will review in next few day. But my intuition said maybe we can just set font and scale anyway, the if guard may be unnecessary.

@ChristianCelora

ChristianCelora commented Mar 14, 2022

Copy link
Copy Markdown
Contributor Author

Hi,

do you had a chance reviewing the PR?

I tried to remove the if guard myself but when i try to open guake for the first time it returns an error on the self.terminal being equals to None.

Here the logs printed during the test run:

INFO     Guake not running, starting it
INFO     Loading Gnome schema from: /home/christian/Documents/python/personal/guake/guake/data
INFO     Language previously loaded from: /home/christian/Documents/python/personal/guake/guake/po
INFO     Guake Terminal 3.8.6.dev1
INFO     VTE 0.60.3
INFO     Gtk 3.24.20
INFO     created fresh notebook for workspace 0
INFO     Spawning new terminal at /home/christian
INFO     current workspace is 0
WARNING  can't bind show-focus key
INFO     Guake tabs restored from /home/christian/.config/guake/session.json
INFO     Guake initialized
INFO     Spawning new terminal at /home/christian
Traceback (most recent call last):
  File "/home/christian/Documents/python/personal/guake/guake/guake_app.py", line 583, in show_hide
    self.show()
  File "/home/christian/Documents/python/personal/guake/guake/guake_app.py", line 743, in show
    self.restore_pending_terminal_split()
  File "/home/christian/Documents/python/personal/guake/guake/guake_app.py", line 672, in restore_pending_terminal_split
    root.restore_box_layout(box, panes)
  File "/home/christian/Documents/python/personal/guake/guake/boxes.py", line 262, in restore_box_layout
    box = box.split_h()
  File "/home/christian/Documents/python/personal/guake/guake/boxes.py", line 460, in split_h
    return self.split(DualTerminalBox.ORIENT_V)
  File "/home/christian/Documents/python/personal/guake/guake/boxes.py", line 485, in split
    terminal.set_font(self.terminal.font)
AttributeError: 'NoneType' object has no attribute 'font'

mlouielu
mlouielu previously requested changes Mar 15, 2022
View reviewed changes

@mlouielu mlouielu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Everything looks fine, but 2 points need to be address:

  1. The block comment for if statement should put under it
  2. Commit message should be polished, please try to use git rebase -i to edit them, and force-push to GitHub.

Comment thread guake/boxes.py Outdated
@ChristianCelora

ChristianCelora commented Mar 21, 2022

Copy link
Copy Markdown
Contributor Author

Hi,

i tried to rebase the commits. I did pick the first one and squashed the other two into the first one.

I am not an expert on git commands (also really scared of using them 🤣). Tell me if i did something wrong.

Christian.

Davidy22
Davidy22 previously approved these changes Apr 7, 2022
View reviewed changes

@Davidy22 Davidy22 left a comment •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I left this to louie since they were on it already and I was busy at the time, but looks like no reply in a little while and I'm digging through stuff on the repo again. Your commit log has three commits, one initial implementation, a reversion and a final implementation and I believe you could clean it up by just dropping the first two commits, but I also don't have a problem with squashing on pull and the change looks fine to me too so I'll do that.

EDIT: Just kidding, apparently squash and merge is just straight disabled here so I am going to have to request a little commit log cleanup. You can either squash the last three commits in your branch, or drop the first two and that'll get us to the clean log that we're looking for.

@ChristianCelora
Fix split terminal.
42aed5b 
Now it preverse the font scale during a vertical / horizontal split.
Resolve issue Guake#1747.
@ChristianCelora

ChristianCelora commented Apr 8, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I polished the commits, squashing them into one single commit, but now the deepsource analysis fails at a commented line that i did not touch, asking to remove it.

Do i have to remove it?

@Davidy22

Davidy22 commented Apr 9, 2022

Copy link
Copy Markdown
Collaborator

I got another pr to sweep up the stuff that deepsource has recently decided to start complaining about, don't worry about that. Looks good, merging.

@Davidy22
Davidy22 merged commit 08f32aa into Guake:master Apr 9, 2022
@Davidy22 Davidy22 mentioned this pull request Apr 9, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Davidy22 Davidy22 Davidy22 approved these changes
@mlouielu mlouielu Awaiting requested review from mlouielu

Assignees

@mlouielu mlouielu

@ChristianCelora ChristianCelora

Labels

Status: On Going

Projects

None yet

Milestone

3.8.6

Development

Successfully merging this pull request may close these issues.

3 participants

@ChristianCelora @mlouielu @Davidy22