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

Add search to current VM Tab #11

Closed
wants to merge 5 commits into from
Closed

Add search to current VM Tab #11

wants to merge 5 commits into from

Conversation

iacore
Copy link

@iacore iacore commented Mar 5, 2022

Screenshot

Partially solve QubesOS/qubes-issues#6667

@@ -1,3 +1,6 @@
# Glade backup file
*.glade~
Copy link
Member

Choose a reason for hiding this comment

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

This should not be added to git.

Copy link
Author

Choose a reason for hiding this comment

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

Why not? .gitignore is used to ignore build files and temporary files.

from .appmenu import main

if __name__ == "__main__":
main()
Copy link
Member

Choose a reason for hiding this comment

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

No newline at the end of file.

@@ -264,23 +264,23 @@ def exit_app(self):
task.cancel()


from .tests.mock_app import new_mock_qapp
Copy link
Member

Choose a reason for hiding this comment

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

Mocking should not happen in the application code itself, but exclusively in the tests.

Copy link
Author

@iacore iacore Mar 8, 2022

Choose a reason for hiding this comment

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

I don't want to run the app in dom0, and I need a way to run the app.

This feature is off by default: https://github.com/QubesOS/qubes-desktop-linux-menu/pull/11/files#diff-609223c61e23f236f844ac3294812b922d4d09f34be6c552c7b6d43dd5c21d4bR41

Copy link
Member

Choose a reason for hiding this comment

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

Then, create a file in tests to run it with qubesadmin.Qubes() patched. Production code should not import tests under any circumstances.

import logging
import asyncio
import os
import shlex

from watchdog.observers import Observer
Copy link
Member

Choose a reason for hiding this comment

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

Why this change? In what ways is watchdog better than pyinotify?

Copy link
Author

Choose a reason for hiding this comment

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

pyinotify is using asyncore, which is deprecated

@marmarta
Copy link
Member

marmarta commented Mar 5, 2022

Also, @ninavizz , could you review the UX design part here?

@ninavizz
Copy link
Member

ninavizz commented Mar 6, 2022

Oh gosh, I hadn't wanted to add Search for a while—it should be a globally-available thing, and not sit at the individual-VM level. One of the two prototypes had Search in it, and a demonstration of how Search can work when it is implemented.

Discoverability will be a big problem if it's at the individual-Qube level, and what about things that are global—such as settings?

Search Results are very hard, from a UX perspective, and there is so much else to get right in the appmenu, first.

Happy to do a call Monday AM! Will ping you on Signal!

@ninavizz
Copy link
Member

ninavizz commented Mar 6, 2022

(also, I'm not able to check-out code to review it—no idea how—and, I have yet to upgrade my machine to 4.1, which I should really do this upcoming week...)

@ninavizz
Copy link
Member

ninavizz commented Mar 6, 2022

Ok, hi, I have snowboarding brain—I just now noticed this is a contributor contribution, not a Marta contribution!

@locriacyber the plan for Search has/had been to add it in after many other features. But, we can obvs hasten that if a contributor wants to help with that, first! I commented on the issue, and offered to do a call if you'd like. I have a corporate job 9-5, but am happy to work around your needs to meet up to chat this through. TY for your interest in helping make this tool awesome!

@iacore
Copy link
Author

iacore commented Mar 8, 2022

Also, @ninavizz , could you review the UX design part here?

There is no design thought put into it what-so-ever. I'm not good at GTK or Glade. I only know how to do full text search properly.


@ninavizz the whole search funtionality is in this pull request; it's simple to add to other places as well (look for function edit_distance in my branch). Having a search bar is good for me as for UX.

@iacore
Copy link
Author

iacore commented Mar 15, 2022

@ninavizz emailed you about meet up a week ago (to the address with qubes domain name). Maybe check your spam filter?

@ninavizz
Copy link
Member

@locriacyber Hiya! I just started a new job and have been underwater for the past several weeks, my apologies for leaving you hanging. :(

My primary concern, is that the Search functionality in this PR is on a per-qube basis. Preferably it would be at the parent level of the menu, and query all qubes—the way the current Search App does.

The display of search results, and the type-ahead component, is where the UX of Search can get tricky/hard. And yes, it is also a goal/interest of mine, to see the frontend of the new app menu get to a point where it is visually "elegant" and matches the Figma prototypes. I don't code at all, so we're all depending on folks skilled at GTK to tend to that part.

I will check my Qubes email today, tho, and get back to you asap. Sorry, again, for leaving you hanging!

@marmarek
Copy link
Member

The search got implemented in #16

@marmarek marmarek closed this Dec 20, 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.

4 participants