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 actions" widget #3449

Merged
merged 29 commits into from
Jan 19, 2023
Merged

"Search actions" widget #3449

merged 29 commits into from
Jan 19, 2023

Conversation

dogboydog
Copy link
Contributor

@dogboydog dogboydog commented Aug 12, 2022

This widget can be used to quickly search actions by name, including those from scripts. Click an action in the results triggers it.

action_search.mp4

Some items I'd like input on so that I could try to implement them (maybe if you could point me to relevant files/code):

  • Show icons for each action in the list

  • Filter actions based on which are possible with the active asset (don't show map properties when editing a tileset, for example). Also exclude "search actions" itself from the list

  • De-duplicate code between actionsearchwidget and locatorwidget

I'm sure lots of refinement will need to be done before it's ready. I had another branch with an attempt to deduplicate the code, but can't get it back to a state that builds anyway so might as well just start over with that based on your feedback.

@bjorn
Copy link
Member

bjorn commented Aug 12, 2022

First of all, thank you so much for looking into this feature!

De-duplicate code

I'm mentioning this first because I think it has priority over other improvements. I'd suggest to take it one step at a time. The most similar class seems to me to be ActionResultsView, which looks like a copy of ResultsView. So first step could be to see if you can just replace it with the original.

Second, I don't think we need an additional delegate, especially since the delegate is just working with a string and not directly with the ProjectModel::Match. Is ActionMatchDelegate actually changed from MatchDelegate? I realize we'll want to make some changes, like showing the action's icon and shortcut, but I think we can do that by modifying MatchDelegate in a way that it will still also work for showing files (which could also use icons, for example). Icons are usually requested from the model through the Qt::DecorationRole, which should then also be added to MatchesModel::data, of course.

Talking about the MatchesModel, I think it's one class that's better to just duplicate, so ActionMatchesModel can stay. This is because otherwise we'd have to unify the internal storage for matches.

Finally, there is the ActionSearchWidget. I think it should be possible to re-use the existing LocatorWidget, but not by sub-classing since ideally we'll have only a single instance of this widget. So, we could instead introduce a separate virtual class like LocatorSource, and have two instances of this, one FileLocatorSource and an ActionsLocatorSource. These classes would implement the various things which are different between these sources:

  • Expose the QAbstractListModel to be set on the mResultsView.
  • If necessary, expose a MatchDelegate instance (assuming those could be merged but needed different options).
  • Have a virtual setFilterWords that performs the search and sets the result on the model.
  • Have a virtual activate that takes a QModelIndex and implements what that should do.

Since the locator sources don't store any relevant state they can be constructed on-demand along with the LocatorWidget, for example:

    auto source  = files ? new FileLocatorSource : new ActionsLocatorSource;
    mLocatorWidget = new LocatorWidget(source);

The LocatorWidget could take ownership of its source.

Show icons

As mentioned previously, I'd do this the usual way, by supporting Qt::DecorationRole in the model, which needs to return a QIcon from the data function for this role. Then, the delegate would request the icon using this role in its paint function. You could probably use the QIcon::paint function for conveniently painting the icon somewhere.

Filtering based on context

I actually don't know if there is a good way to do this, but I guess you'd have to see if the icon is currently visible, taking into account whether its parent menu is visible. This could be problematic, because actions can be part of multiple menus or also tool bars.

Of course, if we're lucky then Qt provides some way to detect this. It may have some way to know internally because a hidden action no longer triggers based on its shortcut, but the implementation of that might not be exposed in any helpful way.

@bjorn
Copy link
Member

bjorn commented Aug 12, 2022

I think it should be possible to re-use the existing LocatorWidget, but not by sub-classing since ideally we'll have only a single instance of this widget.

Since later on I realized we're instantiating this widget on-demand and deleting it again when it closes, we could actually use sub-classing, but I'd still prefer if the shared part and the polymorphic part were clearly separated, as suggested.

@dogboydog
Copy link
Contributor Author

Thank you very much for the guidance. It will take a while for me to implement this but your posts will help a lot

dogboydog and others added 4 commits November 20, 2022 01:53
* Fixed indentation for ActionSearchWidget.
* Removed unnecessary LocatorMatchesModel base class.
* Moved delegate back to LocatorWidget, until we have a reason to move
  it to the LocatorSource interface.
* Moved splitting of filter text into words back to LocatorWidget.
* Moved LocatorMatch back to ProjectModel::match.
* Removed data members from LocatorSource interface.
* Don't inherit LocatorSource interface from QObject.
Introducing ActionLocatorSource to provide the custom placeholder text,
model and activation logic.
@bjorn
Copy link
Member

bjorn commented Dec 12, 2022

I've finished de-duplicating the code. Now the main issues are the customization that is still needed for the action search, like showing icons and shortcuts, not displaying the action text twice (remnant of reusing file delegate). Whether that is best done by extending the MatchDelegate or by writing a more specific one for action search remains to be seen.

Actions also get penalized in Utils::matchingScore for the need to skip an embedded "&" used for accelerators, but I'm not sure if that's noticeable.

@bjorn
Copy link
Member

bjorn commented Dec 23, 2022

I think this is almost ready to be merged, but I have a few concerns:

  • It seems quite a few actions are shown when they do not apply. I'm tempted to say these are problems for later, but I think it would be good to address the most obvious ones here.

  • Some of the actions crash Tiled. This includes "Flip Horizontally" (since many tools register an action with this ID, it usually triggers an action on an inactive tool) and also "Add Folder to Project..." crashes for some reason after the file has been chosen (we may need to close the locator widget before triggering the action somehow). Since this change makes it so easy to trigger these crashes, it would also be good to address this problem.

  • Finally, we of course need to update the documentation to mention this new action. It should at least be added to the keyboard shortcuts page.

Unfortunately, with the holidays starting now, I won't have much time until next year, but I think there is no good reason to rush this in now.

@dogboydog
Copy link
Contributor Author

Makes sense to me. I'll try to help how I can.
I added a line to the keyboard-shortcuts.rst file. I was looking around the docs site but didn't find a good place to put doc about this yet. But here's a rough draft:

Searching Actions

To search for available actions in Tiled, click View -> Search Actions (Ctrl+Shift+P). The following UI will appear:

image

Enter search terms in the input field to filter the list of actions to those that match. This feature can be used to locate an action without navigating the menu bar. It also provides a way to trigger actions registered from scripts that have not been assigned a keyboard shortcut and have not been added to a menu.

Each entry in the list of results that has a keyboard shortcut will display the shortcut below and to the right of the name of the action. Pressing enter with a result selected or double clicking a result will trigger the action.

For example, if you had a tileset open, and wanted to trigger the action to open the Tileset Properties panel, you can open the Search Actions UI, and enter the search term prop.
image

With the "Tileset Properties" result selected, hit enter, and the Tileset Properties view will open.

A few of the tools register their actions with the same Ids. They do
this because to the user there is only one such action, and they share
the any custom shortcut. When triggering these actions, we need to make
sure to trigger them for the right tool.

Implemented solution is to disable actions from inactive tools, and
search for the first enabled action for each registered Id.
When using Qt 6 on Windows, the LocatorWidget could get destroyed as a
result of activating some actions, like when they open a file dialog.
This caused a crash because then after the action is triggered, close()
was called on a deleted object.

It should be no problem to call close() before triggering the action,
since the actual deletion of the LocatorWidget is delayed so
mLocatorSource should stay valid.
Instead of ignoring the action when the locator widget is already open,
close any existing one and replace it with the new.
Also adjusted the section about creating a new project to the behavior
in Tiled 1.9.
Copy link
Member

@bjorn bjorn left a comment

Choose a reason for hiding this comment

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

Great work getting this feature started and functional @dogboydog! I think it is now polished enough for merging. Further improvements can be made later, like the hiding of some more actions when they are out of context or displaying the menu in which the action resides.

As-is, I think the feature is very helpful and I don't think we have any crashes left to fix (though I didn't try to trigger all actions, heh).

@bjorn bjorn merged commit 7f38dc5 into mapeditor:master Jan 19, 2023
@dogboydog
Copy link
Contributor Author

Thank you so much Bjorn. I'm sorry i couldn't get further on it, I hit a wall there with my skill level. Looking forward to using the final version -- been using a CI build with the feature enabled for a few weeks

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.

2 participants