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

Updates to command-line arguments #2013

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

flightmansam
Copy link
Contributor

@flightmansam flightmansam commented Jun 14, 2022

Discussed in #2008.

I would like to add a -t argument to request specific tasks from the command-line. This may be an opportunity to look at the code structure of how min handles command-line arguments to avoid confusion, inadvertent bugs, and easier extension later on. One opportunity could be to use a specific library like commander.js.

This branch has some uglies at the moment due to my agile desire to just have the feature I want:

  • I use the searchAndSortTasks and moveToTaskCommand from customBangs.js, so this should be separated out (probably to task.js @PalmerAL?)
  • BrowserUI ipc 'switchToTask' and BrowserUI.addTab() share commonality so it should be separated out?
  • Currently the -t argument tries to handle both a task name or a task id by doing an "is all digits?" comparison. Probably should eliminate this ambiguity by having a switch for a task id (maybe --task-id)
  • Would like to have a command line way of switching to a particular tab (maybe -tab, and changing -t to be -task).

Now calling min with the -t argument and a query will try and add the new tab into a specific task (or create one)

Todos:
- Refactor search and sort tasks and the move/switch to task to a more general location of the souce code as it is no longer just used for customBangs

NB: this commit includes the electron API change to app.requestSingleInstanceLock(process.argv) in main which fixes a bug with chromium scrambling of second instance commandline args.
Added ability to pass in the task ID to the query
Added a switch to task IPC
Copy link
Collaborator

@PalmerAL PalmerAL left a comment

Choose a reason for hiding this comment

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

Good idea!

I use the searchAndSortTasks and moveToTaskCommand from customBangs.js, so this should be separated out (probably to task.js)

I think the moveToTaskCommand call can be removed (explained in comments). Separating searchAndSortTasks does seem like a good idea, it could either go in a file in js/util or maybe a method on tasks?

// check for -t task query
if (argv.includes('-t') && argv.indexOf('-t') > 0){
// query for specific task to add search to
initTaskQuery = argv[argv.indexOf('-t') + 1]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can assume that IPC messages will be received in the order they're sent, in which case we can just send a switchToTask IPC here, and then leave the code below as switchToTab without any task information. That should simplify this a fair amount.

@@ -367,14 +382,14 @@ app.on('continue-activity', function(e, type, userInfo, details) {
}
})

app.on('second-instance', function (e, argv, workingDir) {
app.on('second-instance', function (e, argv, workingDir, additionalData) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

argv here should be the arguments of the second instance already - why do they need to be passed in with additionalData?

// if there is no result, need to create a new task
let task

if (/^\d+$/.test(data.taskQuery)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe this would be simpler / better as:

if (tasks.get(data.taskQuery)) {
    task = tasks.get...
} else {
    task = searchAndSortTasks...
}

if (!task) {
    task = tasks.get(tasks.add(...
}

So task IDs would work, but any number that's not a task ID would be used for search or to create a new task.

task.name = data.taskQuery
}

moveToTaskCommand(task.id)
Copy link
Collaborator

Choose a reason for hiding this comment

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

IIUC, this is creating a tab in the current task, then using moveToTaskCommand to move it to the new task. This should just be a switchToTask call before the addTab call instead.

(However, if we send a switchToTask IPC from the main process, as I suggested in my other comment, this whole block of code should go away anyway).

@flightmansam
Copy link
Contributor Author

Sorry I have done nothing more on this topic! Not a very high priority for me at the moment ahah.

@PalmerAL
Copy link
Collaborator

No worries!

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