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

Add input arguments to the applications #66

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

Conversation

RodrigoATorres
Copy link

Problem:
I felt that would be nice to have the possibility to have my browser, file explorer or VScode to open with the tabs I want, or in the folder I want, when restoring a session.
Saving that information automatically when saving a session looked like a hard tasks, and even harder to make it generic for all applications (maybe impossible).

Solution:
The workaround I found for these would be to enable user to manually put arguments on the json files to be passed as input arguments when starting the applications. Adding this functionality itself was not a problem, but an other issue came up:
When the session has more than one instance of the same application, it was impossible to tell which window of the application was opened with which input arguments (I could not find a way, at least).

To overcome that, bigger changes had to be done on the code. I created a function "_startAndWaitPrograms" that would, in case there were instances of the same applications with different input arguments, wait for the window of each instance of the application to open, before starting the other one. The new code is commented, in order to explain exactly how it is done.

How to use:
I tested the code with different applications,. To make use of the new functionality the field "executableArgs" has to be added to the json file.

For google chrome you can add:
"executableArgs": "--new-window gmail.com google.com"

For firefox:
"executableArgs": "--new-window gmail.com"
or (if you want to open more than one tab):
"executableArgs": " gmail.com google.com"

For nautilus:
"executableArgs": "/home/rodrigo/Desktop"

For Vscode:
"executableArgs": "/home/rodrigo/Desktop"

Concerns regarding the code:

  • There are some functions that are not being used anymore, as I adapted them into the "_startAndWaitPrograms" function. Should I remove them?
  • As the new function starts, waits and updates the windowId at the same time, I used the CFG.POLL_ALL_MAX_TIMEOUT value multiplied by 10. Should I create a new constant in config for that? Would that be a good value?
  • I understand that this changes require changes on the user usage and some changes on the structure of the code, so I understand if you decide not to incorporate them.
  • I didn't exactly understand the timeout set on "_waitForAllAppsToStart", so I didn't add it to the new function. Should I add it?

Best regards,

@johannesjo
Copy link
Owner

johannesjo commented Apr 22, 2020

Wow! That's a big one. Thank you very much! I'll review it tomorrow.

Regarding your concerns:

There are some functions that are not being used anymore, as I adapted them into the "_startAndWaitPrograms" function. Should I remove them?

If it's not used then just delete it.

As the new function starts, waits and updates the windowId at the same time, I used the CFG.POLL_ALL_MAX_TIMEOUT value multiplied by 10. Should I create a new constant in config for that? Would that be a good value?

Yes. Makes sense!

I understand that this changes require changes on the user usage and some changes on the structure of the code, so I understand if you decide not to incorporate them.
I didn't exactly understand the timeout set on "_waitForAllAppsToStart", so I didn't add it to the new function. Should I add it?

Making changes to the existing behavior is indeed a bit problematic. There are lots of different scenarios (and the whole x11 stuff is used differently by every app) and chances are that we make the app unusable for some people. I much prefer to add this new behavior as an opt in, via a config flag or a command line argument or both than enforcing it on everybody. Would you be ok with that?

@RodrigoATorres
Copy link
Author

Making changes to the existing behavior is indeed a bit problematic. There are lots of different scenarios (and the whole x11 stuff is used differently by every app) and chances are that we make the app unusable for some people. I much prefer to add this new behavior as an opt in, via a config flag or a command line argument or both than enforcing it on everybody. Would you be ok with that?

Sure, I think it makes perfect sense to use a flag or command line argument. The new code should work with the previous input, but I think it is a good idea to use a flag, at least until we have it well tested on different systems.

@RodrigoATorres
Copy link
Author

Hey @johannesjo,
I just found a bug and fixed it. I was testing the code with input arguments and forgot to test it without them.
Sorry for that,
I will do more testing today.

@johannesjo
Copy link
Owner

@RodrigoATorres that's great!

Will you also make the adjustments to maintain compatibility?

@RodrigoATorres
Copy link
Author

I can do that.

Right now, the new code is supposed to have backwards compatibility, it should work with previous input files.

But I will add a input flag, so if the the flag is not added, it will run exactly the previous code.
Is that what you were planning?

@johannesjo
Copy link
Owner

But I will add a input flag, so if the the flag is not added, it will run exactly the previous code.
Is that what you were planning?

This would be the best solution for now, I think. This way we can sure we don't break anything while also giving people the option to use it. If we don't encounter any issues, we can switch from opt in to opt out later.

@RodrigoATorres
Copy link
Author

Doing more testing I saw that the code is presenting some instabilities.
I will have to spend some time trying to understand it.
Sorry for making the pull request without properly testing the code.

@johannesjo
Copy link
Owner

No worries :) Take your time!

@RodrigoATorres
Copy link
Author

I have been using the new version, with the input arguments flag, daily for the past two weeks now.

I use always the same input, but it opens different software (like Vscode, chrome, firefox, nautilus) in 3 workspaces.

Most of the time it works as expected. When I run lwsm right after I login, sometimes it won't open some windows or won't move it to the second or third workspace. If I wait something like 10 or 20 seconds this problem won't happen. I tried the previous code on the same conditions and it seems to have the same behavior. I am not sure what causes that.

@johannesjo
Copy link
Owner

Thank you! I'll try to review this tomorrow!

src/index.ts Outdated
@@ -461,12 +471,141 @@ async function _startSessionPrograms(
})
.map(win => {
win.instancesStarted += 1;
return startProgram(win.executableFile, win.desktopFilePath);
return startProgram(
Copy link
Owner

Choose a reason for hiding this comment

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

If I am not mistaken this would lead toisAllowInputArgs being false to have no effect?

Copy link
Author

Choose a reason for hiding this comment

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

Sorry for taking so long to answer,
The code would start the programs with arguments, in case there were input arguments in the json file, if the isAllowInputArgs was set to false.
The effect isAllowInputArgs being set to false would be running _startSessionPrograms instead of _startAndWaitPrograms.
I believe I did that way because the input arguments themselves were not a big change on the code, that could lead to instabilities (like _startAndWaitPrograms).
But, now that you said, I agree that makes much more sense to remove them. I will just pass a empty string as an argument then, so the code will behave exactly as before.

src/index.ts Outdated
// Get the windowIds of all Ids that were previously opened in order to
// avoid these windows from being used isntead of the newly created windows
// (necessary for windows with custom input arguments)
var activeWindows = await getActiveWindowListFlow();
Copy link
Owner

Choose a reason for hiding this comment

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

I would prefer using 'letorconst`.

Copy link
Author

Choose a reason for hiding this comment

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

I agree with you, will change them.

@ChenSun-Phys
Copy link

Just saw this implemented. What's the right way to invoke this behavior so that when my session is restored proper arguments will be passed along? For example, I have quite a few Okular instances open. When I restore my session, ideally the current open PDF's will be opened automatically. Thanks.

@RodrigoATorres
Copy link
Author

Just saw this implemented. What's the right way to invoke this behavior so that when my session is restored proper arguments will be passed along? For example, I have quite a few Okular instances open. When I restore my session, ideally the current open PDF's will be opened automatically. Thanks.

This PR does not generate the input arguments automatically, you have to do it manually editing the json files.
To add it manually you can edit the saved json file and add the executableArgs field object of the corresponding window.

For google chrome you can add:
"executableArgs": "--new-window gmail.com google.com"

For firefox:
"executableArgs": "--new-window gmail.com"
or (if you want to open more than one tab):
"executableArgs": " gmail.com google.com"

For nautilus:
"executableArgs": "/home/rodrigo/Desktop"

For Vscode:
"executableArgs": "/home/rodrigo/Desktop"

Following are samples of how the json should look for Google-chrome and Vscode.

        {
          "windowId": "0x2c0000b",
          ...
          "simpleName": "Google-chrome",
          "executableFile": "google-chrome.desktop",
          "desktopFilePath": "/usr/share/applications/google-chrome.desktop",
          "executableArgs": "--new-window gmail.com"
        },
        {
          "windowId": "0x2e00001",
          ...
          "simpleName": "Code",
          "executableFile": "code.desktop",
          "desktopFilePath": "/usr/share/applications/code.desktop",
          "executableArgs": "/path/to/you/code"
        },

@ChenSun-Phys
Copy link

sorry if i would like to help test, how am I supposed to install the feature branch from RodrigoATorres's repository? I assume it's not pushed to npm source given it's not merged yet? Thanks.

@johannesjo
Copy link
Owner

johannesjo commented Jun 18, 2020

@chensun-hep you should be able to do the following:

git clone https://github.com/RodrigoATorres/linux-window-session-manager some-folder
cd some-folder
yarn # or npm i should also work

node cmd.js save
node cmd.js restore

Not sure if the fork is up to date, but it should at least contain the state visible here.

@RodrigoATorres
Copy link
Author

I have just updated the code with the changes you requested.
I also updated README to include instructions on how to use it and included an empty "executableArgs" to the json file when saving a session, to facilitate its edition.

@johannesjo
Copy link
Owner

Thanks. I'll try to review this, this weekend again.

@johannesjo
Copy link
Owner

Sorry. I had a very busy weekend. I will try to do this on the next one. Sorry for the delay :-/

Repository owner deleted a comment from freetoken-Tg1483 Sep 13, 2020
@johannesjo
Copy link
Owner

Sorry! I totally forgot about this one! I'll try to have another look tomorrow.

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.

3 participants