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

Custom shortcuts on commands #1456

Closed
bjorn opened this Issue Feb 28, 2017 · 13 comments

Comments

Projects
None yet
2 participants
@bjorn
Owner

bjorn commented Feb 28, 2017

While customizable shortcuts in general would be nice (#215), there is a special case for commands. Users may set up an arbitrary number of them, but at the moment there is only one hardcoded shortcut (F5) for running the first enabled one.

It would be nice if custom shortcuts could be set in the Edit Commands dialog. For this, a separate (not in the table view) QKeySequenceEdit could be added. These shortcuts then need to be registered on the QMainWindow, probably by instantiating QShortcut instances that will map to the custom commands.

http://discourse.mapeditor.org/t/more-shortcuts-for-commands/2082

@bjorn bjorn added the feature label Feb 28, 2017

@ketanhwr

This comment has been minimized.

Show comment
Hide comment
@ketanhwr

ketanhwr Mar 1, 2017

Contributor

@bjorn, I've started working on this. Anything I should know beforehand?

Contributor

ketanhwr commented Mar 1, 2017

@bjorn, I've started working on this. Anything I should know beforehand?

@bjorn

This comment has been minimized.

Show comment
Hide comment
@bjorn

bjorn Mar 1, 2017

Owner

@ketanhwr You can interface with the commands using the CommandDataModel and you can find most of the UI functionality in the CommandButton. I think maybe the CommandDataModel should be made available as a singleton, so that the MainWindow can manage a list of QShortcut instances based on it.

Owner

bjorn commented Mar 1, 2017

@ketanhwr You can interface with the commands using the CommandDataModel and you can find most of the UI functionality in the CommandButton. I think maybe the CommandDataModel should be made available as a singleton, so that the MainWindow can manage a list of QShortcut instances based on it.

@ketanhwr

This comment has been minimized.

Show comment
Hide comment
@ketanhwr

ketanhwr Mar 1, 2017

Contributor

Alright, I'll take a look into the flow and try to complete this one :)

Contributor

ketanhwr commented Mar 1, 2017

Alright, I'll take a look into the flow and try to complete this one :)

@ketanhwr

This comment has been minimized.

Show comment
Hide comment
@ketanhwr

ketanhwr Mar 2, 2017

Contributor

So should I add another column in the CommandTreeView for the shortcut of that command and then register that command into the main window?

Contributor

ketanhwr commented Mar 2, 2017

So should I add another column in the CommandTreeView for the shortcut of that command and then register that command into the main window?

@bjorn

This comment has been minimized.

Show comment
Hide comment
@bjorn

bjorn Mar 2, 2017

Owner

So should I add another column in the CommandTreeView for the shortcut of that command and then register that command into the main window?

It can be added separately, in which case it would apply to the currently selected row. Ideally, I think the table should be just a list with the names of the commands with the checkboxes for toggling whether they are active (and possible a column for displaying the shortcut), and all editable details should go in a form to the right of it. This would also make room for adding stuff like editable environment variables or working directory (#941).

For this task, it's enough to add the QKeySequenceEdit along with a label like "Shortcut:" below the table view.

Owner

bjorn commented Mar 2, 2017

So should I add another column in the CommandTreeView for the shortcut of that command and then register that command into the main window?

It can be added separately, in which case it would apply to the currently selected row. Ideally, I think the table should be just a list with the names of the commands with the checkboxes for toggling whether they are active (and possible a column for displaying the shortcut), and all editable details should go in a form to the right of it. This would also make room for adding stuff like editable environment variables or working directory (#941).

For this task, it's enough to add the QKeySequenceEdit along with a label like "Shortcut:" below the table view.

@ketanhwr

This comment has been minimized.

Show comment
Hide comment
@ketanhwr

ketanhwr Mar 2, 2017

Contributor

What's wrong with this?

Screenshot

Contributor

ketanhwr commented Mar 2, 2017

What's wrong with this?

Screenshot

@bjorn

This comment has been minimized.

Show comment
Hide comment
@bjorn

bjorn Mar 2, 2017

Owner

@ketanhwr It's not scalable in terms of adding more configuration to each command. Though if you prefer, feel free to implement the feature like that for now. When done this way, you'd need to instantiate a QKeySequenceEdit as the widget used for editing cells in the last column. If you're lucky this happens automatically based on the data type, but I don't really expect that to be the case.

Owner

bjorn commented Mar 2, 2017

@ketanhwr It's not scalable in terms of adding more configuration to each command. Though if you prefer, feel free to implement the feature like that for now. When done this way, you'd need to instantiate a QKeySequenceEdit as the widget used for editing cells in the last column. If you're lucky this happens automatically based on the data type, but I don't really expect that to be the case.

@ketanhwr

This comment has been minimized.

Show comment
Hide comment
@ketanhwr

ketanhwr Mar 2, 2017

Contributor

Then what exactly do you suggest? I'm not able to understand what you are asking for (the label one).

Contributor

ketanhwr commented Mar 2, 2017

Then what exactly do you suggest? I'm not able to understand what you are asking for (the label one).

@bjorn

This comment has been minimized.

Show comment
Hide comment
@bjorn

bjorn Mar 2, 2017

Owner

Then what exactly do you suggest? I'm not able to understand what you are asking for (the label one).

I was just suggesting to add these widgets right below the table, in a horizontal layout:

QLabel "Shortcut:", QKeySequenceEdit

What I mean about scalability in terms of adding more configuration, what I'm looking for eventually is something that looks more like the External Tools in Qt Creator:

options_150

It can stay a lot simpler in Tiled, but in general I think it should use the same arrangement to make space for extra options (the "Save map before executing" option could then also easily be made per-command for example).

Owner

bjorn commented Mar 2, 2017

Then what exactly do you suggest? I'm not able to understand what you are asking for (the label one).

I was just suggesting to add these widgets right below the table, in a horizontal layout:

QLabel "Shortcut:", QKeySequenceEdit

What I mean about scalability in terms of adding more configuration, what I'm looking for eventually is something that looks more like the External Tools in Qt Creator:

options_150

It can stay a lot simpler in Tiled, but in general I think it should use the same arrangement to make space for extra options (the "Save map before executing" option could then also easily be made per-command for example).

@ketanhwr

This comment has been minimized.

Show comment
Hide comment
@ketanhwr

ketanhwr Mar 4, 2017

Contributor

Alright, so here's what I have in my mind right now:

  • Just like you suggested, there will be a QLabel along with a QKeySequenceEdit just below the CommandTreeView which will display the shortcut for the currently selected command.
  • Change ExtendedSelection to SingleSelection for the CommandTreeView so that we can configure shortcut of each command separately.
  • When the user selects a particular command, the shortcut for that particular command is displayed in the QKeySequenceEdit
  • Initially, only the first command will have F5 as the shortcut and other commands won't have any. The user can edit them.

Is this flow alright? I'm currently facing trouble for registering the shortcut each time it is changed. I'll try to make a pull request quickly!

Contributor

ketanhwr commented Mar 4, 2017

Alright, so here's what I have in my mind right now:

  • Just like you suggested, there will be a QLabel along with a QKeySequenceEdit just below the CommandTreeView which will display the shortcut for the currently selected command.
  • Change ExtendedSelection to SingleSelection for the CommandTreeView so that we can configure shortcut of each command separately.
  • When the user selects a particular command, the shortcut for that particular command is displayed in the QKeySequenceEdit
  • Initially, only the first command will have F5 as the shortcut and other commands won't have any. The user can edit them.

Is this flow alright? I'm currently facing trouble for registering the shortcut each time it is changed. I'll try to make a pull request quickly!

@bjorn

This comment has been minimized.

Show comment
Hide comment
@bjorn

bjorn Mar 6, 2017

Owner

Is this flow alright? I'm currently facing trouble for registering the shortcut each time it is changed. I'll try to make a pull request quickly!

I think it's mostly fine. I don't think you need to change the selection from ExtendedSelection, since the QTreeView has the notion of "current index" that you can use for the shortcut widget. Also, if you keep the Command menu that you're working on updated with the shortcuts, you probably won't have to deal with manually registering the shortcuts since menu item shortcuts are registered automatically.

Owner

bjorn commented Mar 6, 2017

Is this flow alright? I'm currently facing trouble for registering the shortcut each time it is changed. I'll try to make a pull request quickly!

I think it's mostly fine. I don't think you need to change the selection from ExtendedSelection, since the QTreeView has the notion of "current index" that you can use for the shortcut widget. Also, if you keep the Command menu that you're working on updated with the shortcuts, you probably won't have to deal with manually registering the shortcuts since menu item shortcuts are registered automatically.

@ketanhwr

This comment has been minimized.

Show comment
Hide comment
@ketanhwr

ketanhwr Mar 6, 2017

Contributor

Oh, I'll add this over that branch itself then 👍

Contributor

ketanhwr commented Mar 6, 2017

Oh, I'll add this over that branch itself then 👍

@ketanhwr

This comment has been minimized.

Show comment
Hide comment
@ketanhwr

ketanhwr Mar 12, 2017

Contributor

@bjorn if the user enters a shortcut which is already registered somewhere else, it would generate the Ambiguous Shortcut error. What should I do to prevent this? Display another dialog that this shortcut is already registered somewhere else?

Contributor

ketanhwr commented Mar 12, 2017

@bjorn if the user enters a shortcut which is already registered somewhere else, it would generate the Ambiguous Shortcut error. What should I do to prevent this? Display another dialog that this shortcut is already registered somewhere else?

@bjorn bjorn closed this in #1482 Mar 14, 2017

bjorn added a commit that referenced this issue Mar 14, 2017

arcrowel added a commit to kpresler/tiled that referenced this issue Mar 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment