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

search engine can be set to preference #2125

Merged
merged 4 commits into from
Sep 21, 2022
Merged

search engine can be set to preference #2125

merged 4 commits into from
Sep 21, 2022

Conversation

PhantomLel
Copy link
Contributor

For issue #2088

@PhantomLel
Copy link
Contributor Author

why's that failing?

@Davidy22
Copy link
Collaborator

Davidy22 commented Sep 9, 2022

It should be possible to run actions on your own repo so that you don't need to wait for me to come round and hit the approve button on this for CI, I'll look back some time later when I remember to to see the result of this next run

@PhantomLel
Copy link
Contributor Author

Ok thanks looks like I just had to run black

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.

Sorry about the delay, tab got lost in the thicket for a while. The code builds and the feature works as expected, just some details in the comments. I'll let the unrelated modified files slide since that's black and you seperated that into its own commit already. Might be good to give a way for the user to add their own search engine as well, but this probably works.

@@ -135,7 +135,7 @@
<property name="can-focus">False</property>
<property name="halign">start</property>
<property name="margin-top">6</property>
<property name="label" translatable="yes">&lt;span size="18000"&gt;&lt;b&gt;Guake properties&lt;/b&gt;&lt;/span&gt;</property>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typo introduced?

<object class="GtkLabel" id="lb_search_engine">
<property name="visible">True</property>
<property name="can-focus">False</property>
<property name="label" translatable="yes">Search Engine: </property>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Generally don't want to do spacing by just adding a bunch of spaces when doing UI work. I assume you did this to make a UI element line up somewhere, but if you want to do that you should use columns. The spaces are difficult for the translators to understand and match, just strip the spaces and if you want to do a re-layout that can be another pr

guake/prefs.py Outdated
@@ -296,6 +296,10 @@ def on_prompt_on_close_tab_changed(self, combo):
"""Set the `prompt_on_close_tab' property in dconf"""
self.settings.general.set_int("prompt-on-close-tab", combo.get_active())

def on_search_engine_changed(self, combo):
print("VALUE: ", combo.get_active())
Copy link
Collaborator

Choose a reason for hiding this comment

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

We want to use the logging functions over prints so that users can control log levels

@@ -53,8 +53,18 @@ def on_search_on_web(self, *args):
clipboard = Gtk.Clipboard.get_default(self.window.get_display())
query = clipboard.wait_for_text()
query = quote_plus(query)

# the urls of the search engine options
ENGINES = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This dict should probably be declared outside of this function, don't need to recreate this dict every time we open a link

@Davidy22
Copy link
Collaborator

Added a release note file, the patch works, for the future it could do with a way for the user to add their own search engine but this'll do for now I think

@Davidy22 Davidy22 merged commit 50c1944 into Guake:master Sep 21, 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.

None yet

2 participants