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

Allow setting working directory #1580

Merged
merged 10 commits into from Jun 3, 2017

Conversation

Projects
None yet
3 participants
@ketanhwr
Copy link
Contributor

commented May 29, 2017

This addresses #941. I've also split 'Command' to 'Executable' and 'Arguments'. I've shown the design in the issue and one of the reasons for this was ..the "Browse" button to choose the executable currently resets the command line arguments.

A screenshot of what I've done:
tiled-wd

@Ablu
Copy link
Contributor

left a comment

Do you think it would be possible to unify the replacement logic for the command and the working directory? So that all variables can be used in both fields? Of course this needs special handling regarding the quotes...

QString(QLatin1String("%1")).arg(mapPath));
// QProcess::setWorkingDirectory doesn't work with quoted paths

// TODO: %executablepath

This comment has been minimized.

Copy link
@Ablu

Ablu May 29, 2017

Contributor

hm. If there is an issue to track it we do not need the todo here I think...

@ketanhwr

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2017

I was thinking the same, but to extend the custom command system with two separate functions would easy when we add new variables. New variables would normally be unique to Executable or Working Directory. So, unifying them won't be helpful, imo.

@Ablu

This comment has been minimized.

Copy link
Contributor

commented May 29, 2017

Well... I think if we have one central place to add new variables that should be just as easy. (if the quote handling is also done automatically). So I would think of one function which does all the replacement is then invoked for both, the command and the working directory.

@ketanhwr

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2017

Alright, I'll think up of some solution for this. I actually made a function substituteVariables() but discarded it. Nvm, I'll add it again

ketanhwr added some commits May 31, 2017

@ketanhwr

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2017

@Ablu I've done both the things which were left, you can review now. The build is failing due to libtiled-java 😕

// Perform variable replacement
if (Document *document = DocumentManager::instance()->currentDocument()) {
const QString fileName = document->fileName();

finalCommand.replace(QLatin1String("%mapfile"),
finalString.replace(QLatin1String("%mapfile"),
QString(QLatin1String("\"%1\"")).arg(fileName));

This comment has been minimized.

Copy link
@Ablu

Ablu May 31, 2017

Contributor

The identation is broken here by one space now.

return replaceVariables(finalCommand);
}

QString Command::replaceVariables(const QString &string, bool flag) const

This comment has been minimized.

Copy link
@Ablu

Ablu May 31, 2017

Contributor

I think the name flag is not really revealing what the variable is for... Maybe something like quotePaths would be more intuitive?

This comment has been minimized.

Copy link
@ketanhwr

ketanhwr May 31, 2017

Author Contributor

quotePaths doesn't make sense when using %executablepath. Something like workingDirectoryVariables was making sense but it was too long a literal name.

This comment has been minimized.

Copy link
@Ablu

Ablu May 31, 2017

Contributor

Hm... I am talking about the boolean variable here... I do not understand why workingDirectoryVariables would make sens here 🤔. The variable controls whether the variable values are quoted, right? So maybe quoteVariableValues?

This comment has been minimized.

Copy link
@ketanhwr

ketanhwr May 31, 2017

Author Contributor

It also controls %executablepath actually.

This comment has been minimized.

Copy link
@Ablu

Ablu May 31, 2017

Contributor

well... Let's see what the result of discussion about the quoting-handling is first...

finalString.replace(
QLatin1String("%mappath"),
QString(QLatin1String("\"%1\"")).arg(mapPath));
}

This comment has been minimized.

Copy link
@Ablu

Ablu May 31, 2017

Contributor

I think this does not really solve the problem. My idea with having a single function for replacement of the variables was that each variable can be used in both (the command and the working dir) settings. Here you only do special handling for this specific variable. But extending this approach to the other variables does not really fly either i think... If we wrap each replacement with an if we could simply stick to two functions as before. So I think this is better solved by doing a final replacement of " characters with an empty string (after all the previous variables are replaced) maybe?

@bjorn: any opinions here?

This comment has been minimized.

Copy link
@ketanhwr

ketanhwr May 31, 2017

Author Contributor

Yes, while merging into two functions I had the same doubt. What you suggest might work, I'm not sure though.

ketanhwr added some commits May 31, 2017

@ketanhwr

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2017

@Ablu what you suggested works nicely! Have a look now 🙂

}
}

if (flag) {

This comment has been minimized.

Copy link
@ketanhwr

ketanhwr May 31, 2017

Author Contributor

Also, I placed this inside if (Document *document = DocumentManager::instance()->currentDocument()) previously. %executablepath is independent of this therefore I moved it ouside that control.

This comment has been minimized.

Copy link
@Ablu

Ablu May 31, 2017

Contributor

Oh... Now I get what you intended to do with the flag variable... I did not realize that it makes no sense to replace the executablepath variable in the executable path setting... 😁. However, I think that this flag is a bad idea. The replacement of the %executablepath variable can be done in the finalWorkingCommand function. The replacement of " can be then made conditional using a parameter.

This comment has been minimized.

Copy link
@ketanhwr

ketanhwr May 31, 2017

Author Contributor

You mean finalWorkingDirectory, right?

This comment has been minimized.

Copy link
@Ablu

Ablu May 31, 2017

Contributor

yep. Sorry.

This comment has been minimized.

Copy link
@ketanhwr

ketanhwr May 31, 2017

Author Contributor

The replacement of " can be then made conditional using a parameter.

Didn't understand this part.

This comment has been minimized.

Copy link
@Ablu

Ablu May 31, 2017

Contributor

Ok. Let me try to explain what I was thinking about: I would move the logic for executablepath into finalWorkingDirectory and then rename flag to something like quoteVariableValues, which is then false for the working directory case --> quotes are not replaced. Is that more clear?

This comment has been minimized.

Copy link
@ketanhwr

ketanhwr May 31, 2017

Author Contributor

Yes okay.

}
}

if (flag) {

This comment has been minimized.

Copy link
@Ablu

Ablu May 31, 2017

Contributor

Oh... Now I get what you intended to do with the flag variable... I did not realize that it makes no sense to replace the executablepath variable in the executable path setting... 😁. However, I think that this flag is a bad idea. The replacement of the %executablepath variable can be done in the finalWorkingCommand function. The replacement of " can be then made conditional using a parameter.

@bjorn
Copy link
Owner

left a comment

Looks like a nice change overal! I've made some inline comments.

In addition, please make sure that existing commands remain functional as-is. It looks like currently their command will no longer be read out at all, but care should be taken not to break the command setup for existing users. Probably it would be enough to read in the "Command" setting into the new "Executable" setting.


if (mFile.exists() && mFile.isFile()) {
finalWorkingDirectory.replace(QLatin1String("%executablepath"),
QString(QLatin1String("%1")).arg(mFile.absolutePath()));

This comment has been minimized.

Copy link
@bjorn

bjorn May 31, 2017

Owner

Why the part with arg? The result will be exactly the same as mFile.absolutePath(), so you can just pass that directly to replace.

This comment has been minimized.

Copy link
@ketanhwr

ketanhwr Jun 1, 2017

Author Contributor

🤦‍♂

finalWorkingDirectory = replaceVariables(finalWorkingDirectory, false);

QString finalExecutable = replaceVariables(executable);
QFileInfo mFile(finalExecutable); //Check if executable is a path or not

This comment has been minimized.

Copy link
@bjorn

bjorn May 31, 2017

Owner

You mean check if executable is a file? But why is it needed?

The main problem I can see here, is that if the user entered python as executable, then the %executablepath would have to be determined by searching the directories listed in the PATH environment variable. On the other hand, I can't really see this being very useful so I wouldn't mind that part being left out.

This comment has been minimized.

Copy link
@ketanhwr

ketanhwr Jun 1, 2017

Author Contributor

Yes exactly. That's why I left the PATH case out. This was also mentioned in the issue ticket.

This comment has been minimized.

Copy link
@bjorn

bjorn Jun 2, 2017

Owner

I still don't really understand why you need this check. What would go wrong, if you left it out and just always replaced %executablepath with mFile.absolutePath()? And in what way is it more useful for the user to replace it with an empty string instead?

This comment has been minimized.

Copy link
@ketanhwr

ketanhwr Jun 3, 2017

Author Contributor

Aah, didn't think that through.

}
}

return finalCommand;
if (!quotePaths)
finalString.replace(QLatin1String("\""), QString(QLatin1String("")));

This comment has been minimized.

Copy link
@bjorn

bjorn May 31, 2017

Owner

What if any of the replaced variables contained quotes in their replacements? It would be strange to strip those as well. Instead, you could implement this by making the QString(QLatin1String("\"%1\"")) part of all the above replace calls into a variable that is just set to QString(QLatin1String("%1")) when variables shouldn't be quoted.

This comment has been minimized.

Copy link
@ketanhwr

ketanhwr Jun 1, 2017

Author Contributor

I thought the same but couldn't figure out a case where a variable would contain quote. I'll do this just to be safe, though.

@@ -175,6 +215,10 @@ CommandProcess::CommandProcess(const Command &command, bool inTerminal)

connect(this, SIGNAL(finished(int)), SLOT(deleteLater()));

if (!mFinalWorkingDirectory.trimmed().isEmpty()) {
setWorkingDirectory(mFinalWorkingDirectory);
}

This comment has been minimized.

Copy link
@bjorn

bjorn May 31, 2017

Owner

Coding style: Please leave out the { and } for single-line bodies.

@@ -56,6 +62,10 @@ struct Command
*/
QString finalCommand() const;

QString finalWorkingDirectory() const;

QString replaceVariables(const QString &string, bool quotePaths = true) const;

This comment has been minimized.

Copy link
@bjorn

bjorn May 31, 2017

Owner

Judging by the implementation, it seems quotePaths should be called quoteValues.

@bjorn
Copy link
Owner

left a comment

Some more comments. :-)

finalWorkingDirectory = replaceVariables(finalWorkingDirectory, false);

QString finalExecutable = replaceVariables(executable);
QFileInfo mFile(finalExecutable); //Check if executable is a path or not

This comment has been minimized.

Copy link
@bjorn

bjorn Jun 2, 2017

Owner

I still don't really understand why you need this check. What would go wrong, if you left it out and just always replaced %executablepath with mFile.absolutePath()? And in what way is it more useful for the user to replace it with an empty string instead?

mFile.absolutePath());
} else {
finalWorkingDirectory.replace(QLatin1String("%executablepath"),
QString(QLatin1String("")));

This comment has been minimized.

Copy link
@bjorn

bjorn Jun 2, 2017

Owner

In case you leave this here, note that to create an empty string you should just write QString(), and to remove something from a string, you would use finalWorkingDirectory.remove(...).

QString Command::finalCommand() const
{
QString finalCommand = command;
QString finalCommand = QString(QLatin1String("%1 %2")).arg(executable).arg(arguments);

This comment has been minimized.

Copy link
@bjorn

bjorn Jun 2, 2017

Owner

Please use one call to arg, like .arg(executable, arguments).


if (mFile.exists() && mFile.isFile()) {
finalWorkingDirectory.replace(QLatin1String("%executablepath"),
mFile.absolutePath());

This comment has been minimized.

Copy link
@bjorn

bjorn Jun 2, 2017

Owner

Coding style: please align wrapped function arguments (so mFile.absolutePath()); should start at the same position as QLatin1String in the line above).

ketanhwr added some commits Jun 3, 2017

@bjorn bjorn merged commit f59b10c into bjorn:master Jun 3, 2017

1 of 2 checks passed

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

This comment has been minimized.

Copy link
Owner

commented Jun 3, 2017

Looking good, thanks!

@bjorn bjorn referenced this pull request Jun 3, 2017

Merged

Adds Autocrop feature #1574

thabetx added a commit to thabetx/tiled that referenced this pull request Jun 7, 2017

Allow setting working directory (bjorn#1580)
Also separated arguments and executable and added %executablepath variable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.