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

added is_visible returning boolean Guake show/hide status #1935

Closed
wants to merge 1 commit into from
Closed

added is_visible returning boolean Guake show/hide status #1935

wants to merge 1 commit into from

Conversation

argoyal
Copy link

@argoyal argoyal commented Oct 18, 2021

Added is_visible flag that returns the value of hidden variable from the guake instance which is unset and set on show and hide functions respectively.

@argoyal argoyal closed this Oct 18, 2021
@argoyal argoyal reopened this Oct 18, 2021
@argoyal
Copy link
Author

argoyal commented Oct 18, 2021

Hi @argoyal, could you tell us why you close this PR? At first glance it should work as what it should be.

@mlouielu just got confused a bit. Have reopened it back.

@Davidy22
Copy link
Collaborator

Davidy22 commented Oct 18, 2021

For the release notes file, you'll want to actually fill it in as well. Delete the sections that aren't relevant, and fill in the sections that are. I'll test this out in the meantime

@argoyal
Copy link
Author

argoyal commented Oct 18, 2021

For the release notes file, you'll want to actually fill it in as well. Delete the sections that aren't relevant, and fill in the sections that are. I'll test this out in the meantime

Sure I will make the necessary changes @Davidy22

@argoyal
Copy link
Author

argoyal commented Oct 18, 2021

For the release notes file, you'll want to actually fill it in as well. Delete the sections that aren't relevant, and fill in the sections that are. I'll test this out in the meantime

@Davidy22 do you want me to force push to keep the commit history clean or do you want me to create a separate commit message for these changes.

@Davidy22
Copy link
Collaborator

Force pushing'll be better for once this gets merged

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.

Noticed this got updated a little late. I finally got around to testing this out and it doesn't seem like this works. The point of the requested command is to return true/false values on the terminal for use in other scripts, and this returns nothing.

@@ -578,6 +585,9 @@ def main():
remote_object.show_about()
only_show_hide = options.show

if options.is_visible:
return remote_object.hidden
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like this needs reworking. Return values from main don't go to terminal output, this is python, the return value is just going to go to where main() was called.

Copy link
Author

Choose a reason for hiding this comment

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

okay so rather than a return if I log this output in the shell using logging or print essentially that is what is required, am I correct?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume you created this pull request to resolve #1926. That issue asks for a flag that returns the current visible state of Guake that can be used in shell scripts. This means that the method you use to return the current status must do so in a way that can be useful in a chained line of shell commands.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This didn't meet the need of #1926:

➜  guake git:(pr_1935) ✗ python3 guake/main.py --is-visible
➜  guake git:(pr_1935) ✗ echo $?
0

If the requirement is return 1 & 0, then we should use exit for this.


Also, remote_object.hidden is a dbus.proxies._DeferredMethod, not boolean.

@Davidy22 Davidy22 linked an issue Oct 20, 2021 that may be closed by this pull request
Copy link
Collaborator

@mlouielu mlouielu left a comment

Choose a reason for hiding this comment

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

Didn't meet #1926 needed.

@@ -578,6 +585,9 @@ def main():
remote_object.show_about()
only_show_hide = options.show

if options.is_visible:
return remote_object.hidden
Copy link
Collaborator

Choose a reason for hiding this comment

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

This didn't meet the need of #1926:

➜  guake git:(pr_1935) ✗ python3 guake/main.py --is-visible
➜  guake git:(pr_1935) ✗ echo $?
0

If the requirement is return 1 & 0, then we should use exit for this.


Also, remote_object.hidden is a dbus.proxies._DeferredMethod, not boolean.

@Davidy22
Copy link
Collaborator

This PR has been supplanted by #2008 which works, closing.

@Davidy22 Davidy22 closed this Jan 16, 2022
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.

Add an --is-visible option
3 participants