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

Small stuff #2

Merged
merged 3 commits into from
Jun 16, 2024
Merged

Small stuff #2

merged 3 commits into from
Jun 16, 2024

Conversation

humdingerb
Copy link
Contributor

  • Accept files dropped on the CommandsWindow
  • Add an "Edit" button that opens a command script in the editor
  • Update user guide

Copy link
Owner

@augiedoggie augiedoggie left a comment

Choose a reason for hiding this comment

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

Thanks for working on it! I need to add a better way to get the path from the command string for this as well as the icon lookup code.

Comment on lines 86 to 89
.AddGlue()
.Add(fEditButton = new BButton("Edit" B_UTF8_ELLIPSIS, new BMessage(kEditCommandAction)))
.End()
Copy link
Owner

Choose a reason for hiding this comment

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

I'm still debating about the text of the button. It might be better to be more explicit and say "Edit file". We can probably leave it like this for now until I get around to adding translation support.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think about icon buttons. A folder icon for the "Browse" and a text icon + pen for the "Edit"?
Those could be put to the right of the BTextControl which would free up the space below as a positive side-effect.

Copy link
Owner

Choose a reason for hiding this comment

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

I'm not against having icons but I was more concerned about making it clear that the file would be opened, instead of opening an editor that could be used to edit the current command line arguments. Maybe I'm overthinking it or not giving the users enough credit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure it's really a problem. I think the user will click on the icon and sees what happens and will know now what it exactly does. It's non-destructive, so nothing can really go wrong. Plus we can add a tooltip.

Comment on lines 184 to 240
void
CommandsWindow::_EditCommand()
{
if (_CommandIsScript()) {
const char* argv[] = { fCommandControl->Text(), NULL };
be_roster->Launch("text/x-source-code", 1, argv);
}
}


bool
CommandsWindow::_CommandIsScript()
{
BPath path = fCommandControl->Text();
if (path.InitCheck() != B_OK)
return false;

char mimeType[B_MIME_TYPE_LENGTH];
BNode commandNode(path.Path());
BNodeInfo(&commandNode).GetType(mimeType);
if (strncmp("text/", mimeType, 5) == 0)
return true;

return false;
}

Copy link
Owner

Choose a reason for hiding this comment

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

This is going to cause a problem if fCommandControl has shell escapes/quotes in the command or command line arguments. For example, if the command was /boot/home/myscript.sh --myarg then it would need to have the --myarg removed. I have some really ugly and crude code at

// attempt to find the actual file by splitting the command/arguments at the spaces
that could be moved to its own function and reused but I'd like to find a more reliable way to extract just the file path from the whole command string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haven't thought about that...
It'd be easiest and probably most robust to split that in two BTextControls:

Command/Script:    /boot/home/myscript.sh
Arguments:         --myarg 5038 --myarg2 etc.

Or do you deem that too crude... :)

Copy link
Owner

Choose a reason for hiding this comment

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

Certainly I've thought about splitting it up, and I may end up doing that eventually, but I was really avoiding doing that because I think it makes the user interface more cumbersome to use. Much easier to see or type out a whole command in one input box. I'll have to think about it to see if there are any other consequences of changing it.

Copy link
Owner

Choose a reason for hiding this comment

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

One problem is going to be migrating existing users to the new system. It would be difficult to automatically convert the old commands to the split version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. Good luck with your command/argument parse&split then... :)

@humdingerb
Copy link
Contributor Author

I'm currently having the hardest time with script filename with a space. The path needs to be escaped when actually running the command, but deescaped when launching the script for editing in a text editor (_EditCommand()) and checking if to enable the edit button (_CommandIsScript()).
It's all working well, and I see the correctly deescaped paths when using printf(). (_CommandIsScript() works, but the actual launching of the text editor fails (_EditCommand()).

I'll keep at it for a bit...
Also update the PR with that, in case you see the obvious. The PR as is won't be merged yet anyway, because the commandline parsing to get the pure command without arguments needs to be working first.

@humdingerb
Copy link
Contributor Author

Quick question: I'm only used to our simple makefile_engine makefiles. How can I build a debug version with cmake?

@augiedoggie
Copy link
Owner

augiedoggie commented Feb 21, 2024

I'm currently having the hardest time with script filename with a space. The path needs to be escaped when actually running the command, but deescaped when launching the script for editing in a text editor (_EditCommand()) and checking if to enable the edit button (_CommandIsScript()).

Unfortunately this is only one of many possible variations. It doesn't necessarily even have to do with spaces. I could quote a command like '/boot/home/myscript.sh' --foo and now I have to check to see if the quotes should be removed.

@augiedoggie
Copy link
Owner

Quick question: I'm only used to our simple makefile_engine makefiles. How can I build a debug version with cmake?

cmake -DCMAKE_BUILD_TYPE=Debug should do it.

@humdingerb
Copy link
Contributor Author

thanks!

@augiedoggie
Copy link
Owner

It seems like someone has probably created a regular expression to do the splitting and that's probably the most reliable way to do it. A quick search kept giving me results for splitting inside of bash but that's not what I want. I'll have to do a more thorough search when I get time.

@humdingerb
Copy link
Contributor Author

JFYI, "Edit file..." now works with spaces in filenames. I'm unsure why, but it does after changing

	const char* argv[] = { fCommandControl->Text(), NULL };
	be_roster->Launch("text/x-source-code", 1, argv);

to

	BString file = _Deescape(fCommandControl->Text());
	const char* argv[] = { file.String(), NULL };
	be_roster->Launch("text/x-source-code", 1, argv);

@augiedoggie
Copy link
Owner

It doesn't look like it handles command line arguments though. I can probably come up with other examples that won't get parsed properly and I'd like to have the button working more reliably before it gets put into the app.

@humdingerb
Copy link
Contributor Author

Correct. Once you've solved the hard problem of parsing out the command/script from the arguments - :) - it should be easy to provide _CommandIsScript() and _EditCommand() with the correct path.

@augiedoggie
Copy link
Owner

I haven't forgotten about this but I'm still undecided about the edit button. If you want to split the DnD part into a separate PR then I'll merge it.

@humdingerb
Copy link
Contributor Author

OK, moved the d'n'd to #5.

@augiedoggie
Copy link
Owner

After more thinking, I would probably prefer having a "Show in Tracker" button instead of "Edit File". Once it's clicked and the file is shown in Tracker then the user could edit/rename/copy/whatever...

Would this be something that you want to work on?

* _CommandIsScript() simply checks if the command's path is valid
  and points to a text file.

* _ShowCommand() opens the current script's location in Tracker and
  selects it.

* Only enable the "Show in Tracker" button, if the command is a text file.
  It stays disabled, e.g. for a simple CLI command like /bin/ls.
* Mention the new "Show in Tracker" button and drag'n'drop of a script file
  to set the command.
* Updated screenshot.
* Optimized all PNGs.
@humdingerb
Copy link
Contributor Author

Yes. As it happens, I worked on something similar to "Show in Tracker" for Genio recently. I could therefore very easily adopt that code. :)
I've updated the PR accordingly.

@augiedoggie augiedoggie merged commit 9eef830 into augiedoggie:main Jun 16, 2024
2 checks passed
@augiedoggie
Copy link
Owner

Nice. Thanks for working on it. I'll try to find some time to put out a new release.

@humdingerb humdingerb deleted the smallstuff branch June 16, 2024 08:43
@humdingerb
Copy link
Contributor Author

Very good!
If you're interested in putting up the app at Polyglot, I opened #6.

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