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

Argument parsing rework #1940

Merged
merged 10 commits into from Sep 26, 2020
Merged

Argument parsing rework #1940

merged 10 commits into from Sep 26, 2020

Conversation

zneix
Copy link
Collaborator

@zneix zneix commented Sep 3, 2020

Pull request checklist:

  • CHANGELOG.md was updated, if applicable

Description

Makes use of QCommandLineParser and parses user input adding few dank features:

  • Shows detailed description with --help:
  • Handles joining only channels from command line, for example: chatterino --channels pajlada,zneix,mm2pl will start chatterino, but only with those 3 channels open in each split. I think that comma as split character is fine since it can't be a part of twitch username.
  • To make sure this doesn't overwrite existing configuration in window-layout.json, saving in WindowManager is disabled when application was invoked with --channels argument.

Small TODO:

  • Add documentation
  • Rework handling --channels - it works, but it still uses strings to parse JSON, maybe there's a better solution?

Closes #43


Notes to take a look at before merging:

  1. Everything works fine on linux, however on Windows: --help gives you dialog window with help message, but --version doesn't show anything and quits - that was also a thing before, but we can fix it by using native addVersionOption method, however that will only output app name + version without commit, removing functionality added in Made --version argument output consistent with AboutPage #1739 which I don't wanna do.

Also moved check from BrowerExtension.cpp to Args.cpp as it is more relevant there and doesn't require passing arguments to a function in another file
@zneix zneix marked this pull request as ready for review September 4, 2020 16:42
@ALazyMeme
Copy link
Collaborator

ALazyMeme commented Sep 5, 2020

Closing a stale issue? image

@fourtf
Copy link
Member

fourtf commented Sep 5, 2020

I suppose this should also disable settings from being changed and saved since they are not synced between instances?

Another solution would be to use IPC to add a new window to the original chatterino instance

@zneix
Copy link
Collaborator Author

zneix commented Sep 5, 2020

I suppose this should also disable settings from being changed and saved since they are not synced between instances?

Agree, it is ignoring saving layout to window-layout.json if the application was started with -c flag.

Another solution would be to use IPC to add a new window to the original chatterino instance

Not sure what do you mean here, this doesn't create separate instances of chatterino - it only generates its own layout based on channels supplied in parameter while ignoring layout in window-layout.json.

@fourtf
Copy link
Member

fourtf commented Sep 5, 2020

Agree, it is ignoring saving layout to window-layout.json if the application was started with -c flag.

I mean it might be good to extend this to the regular settings as well, not just the layout of the windows.

Not sure what do you mean here, this doesn't create separate instances of chatterino - it only generates its own layout based on channels supplied in parameter while ignoring layout in window-layout.json.

Right now (unless I am mistaken) running chatterino with --channel x will open a new instance of chatterino instead of communicating to the running instance that it should open the channels in a new window.

pajlada
pajlada previously requested changes Sep 6, 2020
Copy link
Member

@pajlada pajlada left a comment

Choose a reason for hiding this comment

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

The --crash-recovery option should be hidden, see https://doc.qt.io/qt-5/qcommandlineoption.html#Flag-enum

As fourtf mentioned in the comments about, when chatterino has been launched with the --channels option, settings saving should also be disabled.

More open feedback: --channels foo,bar will now open two tabs with foo in the first tab, and bar in the second tab. This assumes Twitch channels, this should probably be specified somehow. And how the channels are opened should probably also be settable somehow.
random thoughts:
--twitchchannels foo,bar or --channels twitch:foo,twitch:bar
Maybe making the character inbetween change whether something opens in a new tab or not?
So maybe --channels foo+bar is a single tab with foo and bar horizontally split, and --channels foo/bar is vertically split? Not sure what characters are best to use.

If the "changing how channels are set up" ends up being too big, you're fine to implement that in a separate PR imo, just give a bit of thought to it so the , character can stay the same behaviour

@zneix
Copy link
Collaborator Author

zneix commented Sep 12, 2020

I changed the way how you specify channels - it's now platform-channelName, where platform is just a single letter. I don't know how IRC channel integration in Chatterino works, so I didn't implement those, but that can be easily added if there'll be need for it.

Also changed tab delimeter character to ; because of following:
As for opening more splits in one tab, I thought of a following idea: --channels t-foo,t-bar;t-asd,t-test would open 2 tabs with 2 splits each:

  1. containing Twitch channels foo and bar
  2. containing Twitch channels asd and test

This could be done, but handling multiple vertical and horiziontal splits in one tab is horrible and at this point I think it's a bit overcomplicated, so I don't know if I should try implementing multiple splits in one tab with more parsed JSON or just drop that altogether, wait for eventual rework of WindowManager.cpp and then maybe even remove parsed JSON.

@pajlada
Copy link
Member

pajlada commented Sep 13, 2020

would prefer to use a character other than -, which could be part of a username atm.
maybe : instead?
and if no prefix is supplied, having Twitch be the default seems sane for this Twitch chat client

pajlada added a commit that referenced this pull request Sep 13, 2020
Previously, we were creating UI elements at while we were reading the window-layout.json file.
We now read the window-layout.json file fully first, which results in a
WindowLayout struct which is built up of a list of windows with a list
of tabs with a root node which contains containers and splits.
This WindowLayout can then be applied.

This will enable PRs like #1940 to start Chatterino with Window Layouts
that aren't defined in a json file.

This commit has deprecated loading of v1 window layouts (we're now on v2). If a v1 window layout is there, it will just be ignored and Chatterino will boot up as if it did not have a window layout at all, and on save that old window layout will be gone.
pajlada added a commit that referenced this pull request Sep 19, 2020
…1964)

* Split up Window Layout loading into a loading and application stage

Previously, we were creating UI elements at while we were reading the window-layout.json file.
We now read the window-layout.json file fully first, which results in a
WindowLayout struct which is built up of a list of windows with a list
of tabs with a root node which contains containers and splits.
This WindowLayout can then be applied.

This will enable PRs like #1940 to start Chatterino with Window Layouts
that aren't defined in a json file.

This commit has deprecated loading of v1 window layouts (we're now on v2). If a v1 window layout is there, it will just be ignored and Chatterino will boot up as if it did not have a window layout at all, and on save that old window layout will be gone.

* Fix compile error for mac
@mlvzk
Copy link

mlvzk commented Sep 23, 2020

I tried this and it works well, I use it in a script that runs streamlink and chatterino.

@zneix
Copy link
Collaborator Author

zneix commented Sep 23, 2020

Yeah, it is working fine with current code however I should get rid of JSON parsing and it is possible because #1964 is merged now. I just gotta rework that one bit and it will be good to go. I have to find some time in the week and I hope I can make it before this weekend. Thanks for approving though :)

@fourtf fourtf dismissed pajlada’s stale review September 26, 2020 13:53

I wanna merge it

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.

Commandline options
5 participants