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

bugfix: sessions save error and some warnings from make tests #1781

Closed
wants to merge 7 commits into from

Conversation

zeusintuivo
Copy link

@zeusintuivo zeusintuivo commented Jul 10, 2020

Related to fix to these bugs:
[x] #1775
[x] #1718
[x] #1737
[x] #1739
[x] #1753
[x] #1777
[x] #1779

[x] Fixes reading session.json file.

@zeusintuivo zeusintuivo marked this pull request as draft July 10, 2020 02:16
@zeusintuivo zeusintuivo marked this pull request as ready for review July 10, 2020 13:50
Copy link
Member

@ulidtko ulidtko left a comment

Choose a reason for hiding this comment

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

Hi! Please fill out the changelog (releasenotes) stub file, it's no good as it stands currently.

Comment on lines +1 to +57
release_summary: >
Replace this text with content to appear at the top of the section for this
release.

All of the prelude content is merged together and then rendered
separately from the items listed in other parts of the file, so the text
needs to be worded so that both the prelude and the other items make sense
when read independently.

Do not use "list" syntax here

features:
- |
List new features here followed by the ticket number, for example::

- new exiting feature #1234

known_issues:
- |
List know issue introduced by the change here, followed if possible by a
ticket number, for example::

- such other feature is broken #1234

upgrade:
- |
List upgrade note for end users here

deprecations:
- |
List deprecations notes heres, ie, feature that are being removed by the
change.

security:
- |
Add security notes here.

fixes:
- |
Add normal bug fixes here, followed by the ticket number, for example::

- broken feature has been fixed #1234

translations:
- Only put a list of updated 2 letters language code, for example::

translations:
- fr
- de

notes_for_package_maintainers:
- |
Add notes for package maintainers here.

other:
- |
Add other notes here.
Copy link
Member

Choose a reason for hiding this comment

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

You're supposed to fill out this file and delete irrelevant parts.

Copy link
Collaborator

@Davidy22 Davidy22 left a comment

Choose a reason for hiding this comment

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

There's a lot going on in here, made some comments. The slug file still needs filling in, also we have an updated CI pipeline now so you'll want to update and make sure your code passes the build. Also consider splitting this up, there's a big grab-bag of issues that you've listed that this fixes.

super(PromptQuitDialog, self).__init__(
parent,
transient_for,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not completely sure why this is renamed here

filteru.set_name(_("Text and Logs"))
filteru.add_pattern("*.log")
filteru.add_pattern("*.txt")
self.add_filter(filteru)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did we rename filter to filteru here?

@@ -101,10 +101,10 @@
# Disable find feature until python-vte hasn't been updated
enable_find = False

GObject.threads_init()
# GObject.threads_init()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Git keeps a record of old code so we don't have to comment code out like this

@@ -289,7 +289,7 @@ def delete_page(self, page_num, kill=True, prompt=0):
procs = self.get_running_fg_processes_count_page(page_num)
if prompt == 2 or (prompt == 1 and procs > 0):
# TODO NOTEBOOK remove call to guake
if not PromptQuitDialog(self.guake.window, procs, -1).close_tab():
if not PromptQuitDialog(self.guake.window, procs, -1, notebooks="").close_tab():
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this not already the default value for notebooks here?

@@ -461,7 +463,7 @@ def on_use_audible_bell_toggled(self, chk):
"""
self.settings.general.set_boolean("use-audible-bell", chk.get_active())

# scrolling tab
# scrolling tab
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why the space

{"key": "move-terminal-split-down", "label": _("Move the terminal split handle down"),},
{"key": "move-terminal-split-up", "label": _("Move the terminal split handle up"), },
{"key": "move-terminal-split-down",
"label": _("Move the terminal split handle down"), },
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did this file need the reformatting?

def on_background_image_layout_mode_changed(self, combo):
val = combo.get_active()
self.settings.general.set_int("background-image-layout-mode", val)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this background image code? Related to #1604?

@Davidy22
Copy link
Collaborator

Coming back to this, most of the tickets that this pull request closes look like they're already closed, so this pr currently I believe is just a bunch of collisions with existing fixes and one fix for a still outstanding bug #1779. I'll give you some time to remake this pull request as just the fix to that bug, and then I'll close this one, I'll do it myself if I get around to it.

Davidy22 added a commit to Davidy22/guake that referenced this pull request Oct 1, 2021
@Davidy22
Copy link
Collaborator

Davidy22 commented Oct 1, 2021

I got the time, so I did it myself. New pr with the last remaining change at #1917, closing this one.

@Davidy22 Davidy22 closed this Oct 1, 2021
gsemet pushed a commit that referenced this pull request Oct 5, 2021
PhungXuanAnh pushed a commit to PhungXuanAnh/guake that referenced this pull request Oct 28, 2023
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.

3 participants