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 an executable picker for setting up commands #942

Closed
bjorn opened this Issue May 12, 2015 · 9 comments

Comments

Projects
None yet
2 participants
@bjorn
Owner

bjorn commented May 12, 2015

It would be convenient if a file picker would be available for finding the executable to execute for a command.

Requested at issue #940.

@bjorn bjorn added the feature label May 12, 2015

@ketanhwr

This comment has been minimized.

Show comment
Hide comment
@ketanhwr

ketanhwr Mar 15, 2017

Contributor

@bjorn I'd like to work on this one now. What would you suggest regarding the UI? I am thinking something along the lines of how custom shortcuts were registered:

  • Add a text edit above the QKeySequenceEdit and the user can enter the command there, instead of entering it in the CommandTreeView.
  • Just above the Clear button, there will be another QPushButton which would be an option to select the file of an executable.

Does this UI seem fine?

Contributor

ketanhwr commented Mar 15, 2017

@bjorn I'd like to work on this one now. What would you suggest regarding the UI? I am thinking something along the lines of how custom shortcuts were registered:

  • Add a text edit above the QKeySequenceEdit and the user can enter the command there, instead of entering it in the CommandTreeView.
  • Just above the Clear button, there will be another QPushButton which would be an option to select the file of an executable.

Does this UI seem fine?

@bjorn

This comment has been minimized.

Show comment
Hide comment
@bjorn

bjorn Mar 15, 2017

Owner

I think it makes sense to reorganize the UI a little rather than adding more widgets to the bottom, like in the screenshot I showed you here.

Owner

bjorn commented Mar 15, 2017

I think it makes sense to reorganize the UI a little rather than adding more widgets to the bottom, like in the screenshot I showed you here.

@ketanhwr

This comment has been minimized.

Show comment
Hide comment
@ketanhwr

ketanhwr Mar 15, 2017

Contributor

So I'm trying the following:

Screenshot

The CommandTreeView will have only 1 column now, which will display the name of the column. The user can double click on <new command> to create a new command. The description is meant for Tool Tip text probably. The user can either enter the command directly, or browse by clicking on the Browse... button.

I've reduced the CommandTreeView to only column, because it would seem very odd to display all the data two times.

Is this alright?

Contributor

ketanhwr commented Mar 15, 2017

So I'm trying the following:

Screenshot

The CommandTreeView will have only 1 column now, which will display the name of the column. The user can double click on <new command> to create a new command. The description is meant for Tool Tip text probably. The user can either enter the command directly, or browse by clicking on the Browse... button.

I've reduced the CommandTreeView to only column, because it would seem very odd to display all the data two times.

Is this alright?

@bjorn

This comment has been minimized.

Show comment
Hide comment
@bjorn

bjorn Mar 15, 2017

Owner

That's roughly what it should be, yes. A few points of feedback:

  • I see no need to add a "Description" field at this point.
  • Please move the "Enabled" checkbox back to the list view. You could move the check state into the Name column, as done with the list of plugins in the Preferences for example. Having it in the list is important because that's the only way to quickly see which commands are enabled.
  • Please keep the read-only "Shortcut" column in the list view, for the reason why we put it there.
  • The "Save map before executing" option can be moved where you have the "Enabled" checkbox. Of course, it should be changed to be stored for each command then.
  • Please put a Spacer at the bottom of the vertical layout on the right, to make sure the items sit together instead of getting stretched out.
Owner

bjorn commented Mar 15, 2017

That's roughly what it should be, yes. A few points of feedback:

  • I see no need to add a "Description" field at this point.
  • Please move the "Enabled" checkbox back to the list view. You could move the check state into the Name column, as done with the list of plugins in the Preferences for example. Having it in the list is important because that's the only way to quickly see which commands are enabled.
  • Please keep the read-only "Shortcut" column in the list view, for the reason why we put it there.
  • The "Save map before executing" option can be moved where you have the "Enabled" checkbox. Of course, it should be changed to be stored for each command then.
  • Please put a Spacer at the bottom of the vertical layout on the right, to make sure the items sit together instead of getting stretched out.
@ketanhwr

This comment has been minimized.

Show comment
Hide comment
@ketanhwr

ketanhwr Mar 15, 2017

Contributor

Alright! 👍

Contributor

ketanhwr commented Mar 15, 2017

Alright! 👍

@ketanhwr

This comment has been minimized.

Show comment
Hide comment
@ketanhwr

ketanhwr Mar 15, 2017

Contributor

So this is how it looks currently. I'll hopefully make a pull request by today.

Command Dialog

I've added "Save Before Execute" unique to each command. Any other suggestions? :)

Contributor

ketanhwr commented Mar 15, 2017

So this is how it looks currently. I'll hopefully make a pull request by today.

Command Dialog

I've added "Save Before Execute" unique to each command. Any other suggestions? :)

@bjorn

This comment has been minimized.

Show comment
Hide comment
@bjorn

bjorn Mar 15, 2017

Owner

I've added "Save Before Execute" unique to each command. Any other suggestions? :)

I think it looks quite alright, though you shouldn't move the Close button into the vertical layout on the right. It should stay on its own line at the bottom.

Owner

bjorn commented Mar 15, 2017

I've added "Save Before Execute" unique to each command. Any other suggestions? :)

I think it looks quite alright, though you shouldn't move the Close button into the vertical layout on the right. It should stay on its own line at the bottom.

@ketanhwr

This comment has been minimized.

Show comment
Hide comment
@ketanhwr

ketanhwr Mar 15, 2017

Contributor

The Close button was previously like that only, but it looked very weird at that position since the "Save map before executing" check was moved. I tried a few positions for the close button, and found this one alright.

Contributor

ketanhwr commented Mar 15, 2017

The Close button was previously like that only, but it looked very weird at that position since the "Save map before executing" check was moved. I tried a few positions for the close button, and found this one alright.

@bjorn

This comment has been minimized.

Show comment
Hide comment
@bjorn

bjorn Mar 15, 2017

Owner

Yes, it will waste some space on the left, but that is still the right place for it. The layout you showed is a little confusing because it's not immediately clear that the button closes the dialog, rather than just the right side of it.

Owner

bjorn commented Mar 15, 2017

Yes, it will waste some space on the left, but that is still the right place for it. The layout you showed is a little confusing because it's not immediately clear that the button closes the dialog, rather than just the right side of it.

@bjorn bjorn closed this in #1488 Mar 21, 2017

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

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