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

Added an executable picker - Fixes #942 #1488

Merged
merged 9 commits into from Mar 21, 2017

Conversation

@ketanhwr
Copy link
Contributor

commented Mar 15, 2017

This fixes #942

Command Dialog

Firstly, I've modified the layout of the Command Dialog. Also, I've made the option "Save Map before Execute" unique to each command. The user can now either enter the command, or he can browse an executable.

@bjorn

This comment has been minimized.

Copy link
Owner

commented Mar 15, 2017

Please don't center the Close button! :-)

@ketanhwr

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2017

@bjorn status?

@bjorn

This comment has been minimized.

Copy link
Owner

commented Mar 20, 2017

@bjorn status?

@ketanhwr No need for such messages unless something is inactive for weeks and it may seem I have forgotten. I only have two full days to work on Tiled per week, and any activity beyond that is limited and random. Of course I will commit to spending a bit of time every day during GSoC, but I can't do this all the time. Rest assured that each activity here will usually sit in my inbox already until I get around to look at it.

@bjorn
Copy link
Owner

left a comment

See inline comments on the code.

In general, I think the layout could be improved a little. At least the executable and shortcut inputs should align. In your screenshot, it seems they do, but they don't align when I try out your patch. Was the alignment lost somewhere?

return false;
}

void CommandDataModel::setSaveBeforeExecute(const QModelIndex &index, const bool &value)

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 20, 2017

Owner

Please don't pass a bool as const &, just pass it by value. Less code and more optimal.


void setSaveBeforeExecute(const QModelIndex &index, const bool &value);

QString command(const QModelIndex &index) const;

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 20, 2017

Owner

What about making this function return the full Command, like firstEnabledCommand is doing, avoiding the need for all the other getters?

This comment has been minimized.

Copy link
@ketanhwr

ketanhwr Mar 20, 2017

Author Contributor

Overlooked this, thanks!

void CommandDialog::setSaveBeforeExecute(int state)
{
const QModelIndex &current = mUi->treeView->currentIndex();
if (current.row() < mUi->treeView->model()->rowCount(QModelIndex()))

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 20, 2017

Owner

Please add QModelIndex() as the default parameter of the parent for rowCount. It is the default in the base class already, and then we can avoid being needlessly verbose here.

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 21, 2017

Owner

You added the default parameter but didn't remove the now no longer necessary argument here. :-)

This comment has been minimized.

Copy link
@ketanhwr

ketanhwr Mar 21, 2017

Author Contributor

It's behaving quite strangely now. If I remove these parameters, a compilation error shows up :/
I'm trying to figure it out though

This comment has been minimized.

Copy link
@ketanhwr

ketanhwr Mar 21, 2017

Author Contributor

Oh found the error. I'm stupid.

mUi->commandEdit->setText(executableName);
}

void CommandDialog::enableWidgets(const bool enable)

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 20, 2017

Owner

Please merge this function into updateWidgets. I see no gain in having this as a separate method.


void CommandDialog::openFileDialog()
{
QString caption = tr("Select executable");

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 20, 2017

Owner

Window captions should capitalize each word, so "Select Executable".

void CommandDialog::openFileDialog()
{
QString caption = tr("Select executable");
QString dir = tr("/bin");

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 20, 2017

Owner

That is only useful on Linux, and even there it should probably be /usr/bin. I think we should try relying on QStandardPaths::ApplicationsLocation.

QString executableName = QFileDialog::getOpenFileName(this, caption, dir);

if (!executableName.isEmpty())
mUi->commandEdit->setText(executableName);

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 20, 2017

Owner

Maybe it would be useful to automatically add %mapfile? It will be what the user wants most of the time.

@@ -101,7 +147,6 @@ CommandTreeView::CommandTreeView(QWidget *parent)
QHeaderView *h = header();
h->setStretchLastSection(false);
h->setSectionResizeMode(CommandDataModel::NameColumn, QHeaderView::Interactive);
h->setSectionResizeMode(CommandDataModel::CommandColumn, QHeaderView::Stretch);

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 20, 2017

Owner

Please change the NameColumn from Interactive to Stretch.

@ketanhwr

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2017

I thought you might have overlooked this one, so I tagged you, sorry! I'll keep this in mind next time 😃

@bjorn
Copy link
Owner

left a comment

Did you forget about fixing up the widget alignment?

void CommandDialog::setSaveBeforeExecute(int state)
{
const QModelIndex &current = mUi->treeView->currentIndex();
if (current.row() < mUi->treeView->model()->rowCount(QModelIndex()))

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 21, 2017

Owner

You added the default parameter but didn't remove the now no longer necessary argument here. :-)

mUi->keySequenceEdit->setEnabled(false);
mUi->clearButton->setEnabled(false);
mUi->saveBox->setEnabled(false);

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 21, 2017

Owner

Please use a local variable to de-duplicate all these setEnabled calls.

QString executableName = QFileDialog::getOpenFileName(this, caption, dir);

if (!executableName.isEmpty())
mUi->commandEdit->setText(executableName);

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 21, 2017

Owner

What about the idea to automatically append " %mapfile"?

This comment has been minimized.

Copy link
@ketanhwr

ketanhwr Mar 21, 2017

Author Contributor

I think we should append %mapfile only if the user creates a new command.

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 21, 2017

Owner

I think if we don't always add it here, we need to introduce a separate "arguments" field, like in Qt Creator. That way the arguments can default to %mapfile and choosing the executable becomes independent.

@ketanhwr

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2017

Regarding the alignment, it's working fine for me, I don't know why it's working that way for you. Can you attach a screenshot, perhaps? And also, how about adding a keyboard shortcut for Edit Commands... as well? I think it might be a bit useful for people who'll customise their commands when we add more options to the Command Dialog.

@bjorn

This comment has been minimized.

Copy link
Owner

commented Mar 21, 2017

Regarding the alignment, it's working fine for me, I don't know why it's working that way for you. Can you attach a screenshot, perhaps?

selection_154

And also, how about adding a keyboard shortcut for Edit Commands... as well? I think it might be a bit useful for people who'll customise their commands when we add more options to the Command Dialog.

I think the action is too rare to have a default shortcut for it.

@ketanhwr

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2017

Alright, seems weird, I'll look into this.

ketanhwr added 2 commits Mar 21, 2017
@ketanhwr

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2017

I've fixed the UI problem. Still not sure what to do about the %mapfile appending.

@bjorn
Copy link
Owner

left a comment

Don't try to be too fast, @ketanhwr!

mUi->browseButton->setEnabled(enable);
mUi->keySequenceEdit->setEnabled(enable);
mUi->clearButton->setEnabled(enable);
mUi->saveBox->setEnabled(enable);

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 21, 2017

Owner

You know you still need to de-duplicate these lines, right? I mean now you're using a variable, but...

This comment has been minimized.

Copy link
@ketanhwr

ketanhwr Mar 21, 2017

Author Contributor

Kill me already ;_;

mUi->saveBox->setChecked(mUi->treeView->model()->saveBeforeExecute(current));
mUi->commandEdit->setText(mUi->treeView->model()->command(current));
}
else {

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 21, 2017

Owner

Coding style: } else {. Why did you even change it? It was correct before.

if (current.row() < mUi->treeView->model()->rowCount(QModelIndex()) - 1) {
mUi->keySequenceEdit->setEnabled(true);
mUi->clearButton->setEnabled(true);
bool enable = true;

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 21, 2017

Owner

What about:

bool enabled = current.row() < mUi->treeView->model()->rowCount() - 1;
@ketanhwr

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2017

🤦‍♂

@bjorn
bjorn approved these changes Mar 21, 2017

@bjorn bjorn merged commit 1b3bd61 into bjorn:master Mar 21, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bjorn

This comment has been minimized.

Copy link
Owner

commented Mar 21, 2017

Great, thanks!

About the %mapfile, I think ideally we need to separate the executable from the arguments. If you look at the Qt Creator screenshot I showed you in some issue, you see they have done the same. It would allow the current file to be passed to the command by default, independently of the executable. It also makes the "Select Executable" action less destructive, since it won't affect the argument list.

@bjorn

This comment has been minimized.

Copy link
Owner

commented Mar 21, 2017

I've pushed some follow-up tweaks in change 83b26c4.

@ketanhwr

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2017

Oh, I forgot about removing all the getters and setters. Thanks for your help :-)

@ketanhwr ketanhwr deleted the ketanhwr:command-picker branch Mar 21, 2017

@ketanhwr

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2017

Yes, I was pretty impressed by the Qt Creator's custom command menu. I'm planning to implement something pretty similar in Tiled as well, would be a nice thing to add to my proposal, I think!

@bjorn

This comment has been minimized.

Copy link
Owner

commented Mar 21, 2017

Yes, I was pretty impressed by the Qt Creator's custom command menu. I'm planning to implement something pretty similar in Tiled as well, would be a nice thing to add to my proposal, I think!

I could definitely imagine that being part of your proposal. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.