Adds command line arguments (closes #95)

As promised (and discussed) in #95, this is my take on adding command line arguments.

Arguments can be specified on the command line.
Arguments can be given in configuration files (these use argparse's feature, where you can intersperse configuration files with command line arguments). Configuration file names have to start with the at-sign '@'. Configuration files have the format key[: value] and can have comments.
run() can take the same arguments (positional and keyword) as the command line.
Call ryven -h to display all possible arguments (or just see below)

Open questions:

The code to assemble the editor's startup configuration can now be found both in run() (L443) and StartupDialog. Since StartupDialog is only called from run(), I would prefer to have that code just in the run() function.
The available flow themes are hard coded (Ryven.py, L170). Is there an easy way to get all available flow themes?

ToDos:

Set performance mode from command line/config file
Set animations from command line/config file
En/disable info messages from command line/config file

Comments, suggestions, requests are always welcome!!
ryven --help
usage: ryven [-h] [-s] [-n NODE] [-x {basics,matrices}] [-w {dark,light}]
             [-f {toy,darktron,ghost,blender,simple,ueli,pure dark,colorful,pure light,colorful light,industrial,fusion}]
             [-d] [-q QT_API] [-t TITLE]
             [PROJECT]

Flow-based visual scripting with Python with absolute freedom for your nodes.
See https://ryven.org/guide/ for a guide how to program them.

positional arguments:
  PROJECT               the project file or example to be loaded; use "-" for
                        standard input

optional arguments:
  -h, --help            show this help message and exit
  -s, --show-dialog     show start-up dialog (disables "--example", "--flow-
                        theme" and the loading of the project file)

projects:
  -n NODE, --nodes NODE
                        load a nodes packages; (NODE is the path to the node
                        package without "/nodes.py")
  -x {basics,matrices}, --example {basics,matrices}
                        load an example project (do not give PROJECT argument)

display:
  -w {dark,light}, --window-theme {dark,light}
                        set the window theme (default: dark)
  -f {toy,darktron,ghost,blender,simple,ueli,pure dark,colorful,pure light,colorful light,industrial,fusion}, --flow-theme {toy,darktron,ghost,blender,simple,ueli,pure dark,colorful,pure light,colorful light,industrial,fusion}
                        set the theme of the flow view (default: {pure
                        dark|pure light}, depending on the window theme)

debug:
  -d, --debug           display messages on the console
  -q QT_API, --qt-api QT_API
                        the QT API to be used (default: pyside2)
  -t TITLE, --title TITLE
                        changes the window's title (default: Ryven)

configuration files:
  One or more configuration files can be used at any position. • To mark an
  argument to be used as a configuration files, the file name must be
  preceded with the @-sign, e.g<EMAIL_ADDRESS>The later command line
  arguments or configuration file takes precedence over earlier specified
  arguments. • The format is like the long command line argument, but with
  the leading two hyphens removed. If the argument takes a value, this comes
  after a colon, e.g. these three lines are identical: "example: basics" and
  "example=basics". • Comments can be inserted after the hash sign "#".

Copyright (C) 2020-2022 Leon Thomm, licensed under MIT

And Question 3:
Use a default configuration file – if it exists -- in the user's home directory (or even a system-wide one from /etc/?)

Awesome work, simply amazing. This is a beautiful piece of right there code.

The code to assemble the editor's startup configuration can now be found both in run() (L443) and StartupDialog. Since StartupDialog is only called from run(), I would prefer to have that code just in the run() function.

I agree

The available flow themes are hard coded (Ryven.py, L170). Is there an easy way to get all available flow themes?

leon-thomm/ryvencore-qt@af254d7434dc5170ceb1084f97497270e012f5ec (+ leon-thomm/ryvencore-qt@21204fb64d276fcdb934d9cea207e8f1b40269d0) and 137c9051c6a171126ae4a4adaa616449900b21b5 above
some notes:

loading examples didn't work for me, I fixed the load_nodes procedure for required node packages listed in the project file and added option to override them manually, so you can write ryven -x matrices -n std -n linalg to load the matrices example even though the original package paths listed in the project file don't exist on your system, e33549387d830d37537b159ac5aea74cb56278c5
removed case-sensitivity requirement for specifying them leon-thomm/ryvencore-qt@eb80f39f2cec0bfca241823a914995ccbe606eb1
I think the debug (-d) was reversed, it redirected output to Ryven when flag was set, I switched it fc05542e42befa25f4555cb6b3d885c8fa8c8edb

And Question 3: Use a default configuration file – if it exists -- in the user's home directory (or even a system-wide one from /etc/?)

What is the question exactly? Ryven already uses ~/.ryven/ on Linux for projects and node packages. Putting a configuration file there would bring the advantage of easy portability (users can more easily share what they have built in a reliable way)

Glad that you like it :blush:

The code to assemble […]

I agree.

I'll have a look, if it can entirely be moved into the run() function.
Thanks for making the flow themes available (and at the same time update the ArgumentParser)!

loading examples didn't work for me […]

Oops, I must have changed something fundamentally after testing it. So sorry. Thanks for fixing it. I still have to understand, what went wrong.

removed case-sensitivity requirement for specifying flow themes https://github.com/leon-thomm/ryvencore-qt/commit/eb80f39f2cec0bfca241823a914995ccbe606eb1

Shouldn't that be .casefold() instead of .lower() nowadays? (.lower() is also working …)

I think the debug (-d) was reversed, it redirected output to Ryven when flag was set, I switched it fc05542

Well spotted, that escaped me. Thanks.

What is the question exactly?

Aargh, I was not aware of the ryven_dir_path function. I was more thinking along the xdg line (again …) Yes, that is exactly, what I was looking for. I'll update the code, so that a configuration file in ryven_dir_path() is always loaded. If it exists, that is.
BTW XDG says, this file should be in ~/.config/ryven/, so that it does not fill the root directory with hidden files/directories.
I also

Shouldn't that be .casefold() instead of .lower() nowadays? (.lower() is also working …)

true, good to know, I changed it

BTW XDG says, this file should be in ~/.config/ryven/, so that it does not fill the root directory with hidden files/directories.

Yes, but I'm not sure what's better. On Linux I would agree that the config file should probably be in ~/.config/ryven. However, I am not aware of equally consistent conventions for Windows (and I have no clue about MacOS), and therefore it might be easier to keep everything in one place (that is, putting the config file in ~/.ryven/ because that directory is used anyway for nodes and projects which imo should not be stored in .config) for the sake of platform independence / portability. What do you think?

As far as I understand it, configuration files should go into ~/.config/ryven/ (or ~/.config/ryven.cfg, but I believe the first one is preferred), the nodes into ~/.local/share/ryven and projects wherever the user wants to put them. That's actually where xdg comes to the rescue …

aah, please take over my commits next time, this creates a huge mess when trying to navigate the commits, now I cannot be sure which of my changes are now part of it (and how) and which are not without going through everything again (unless you really carefully copied everything 1:1). you should just need to pull from origin, this PR is basically your argparse branch

I pulled your changes and I'm afraid it does seem to not work; ryven -x basics -n std returns
Error: Nodes packages not found: C:\Users\nutri\projects\ryven projects\Ryven_mmorys\Ryven\ryven\example_nodes\std

it did work with my changes :cry:

aah, please take over my commits next time

I am so sorry that my git-fu failed me. I tried to pull your changes to my local repository, but they failed to materialize here. What would be the right procedure to get hold of them?
I copied all but fixed required package loading from project file paths https://github.com/leon-thomm/Ryven/commit/e33549387d830d37537b159ac5aea74cb56278c5 verbatim.
I try to understand, how a nodes package can be specified:

By the path to nodes.py.
By its name – then the package is searched for in Ryven's package and example_nodes directories.

This is implemented in load_nodes() (or as it is called now: process_nodes()) for nodes packages specified on the command line: It looks, if a nodes.py file exists in the specified path; or if not found, it looks in Ryven's directories for packages with the same name as the specified path.
I wonder if searching in Ryven's directories should be restricted to names without any separators in it to avoid (unlikely) unexpected results?
Should it also look in ryven_dir_path()?
How should nodes packages specified in project files be handled? These have a name and a dir, which is a full path. Here the behaviour is not so clear cut. The graphical interface StartupDialog looks for packages in the specified path; if they cannot be found, the user is presented with the choice to automatically look for these packages in Ryven's directories based on the name. That is very convenient, but how should that be handled on the command line?
One way is to not handle it at all; starting the main window would then fail with an error message listing the missing packages. The user then can supply them on the command line, either with their full path or just their name, if they are available in Ryven's directories.
Alternatively the command line program can automatically search for the missing packages based on their names.
A switch can be added to switch between these two behaviours. Which should be the default?
BTW: At the moment my code does not distinguish between the name and the dir attribute in the project's file. Should it try to load the package by the file name and if failing use the name attribute to locate the package in Ryven's directories? Or – alternatively – use the last part of dir as package name?

What would be the right procedure to get hold of them?

something like
git checkout argparse
git pull origin

should work. I usually use a client like GitKraken or GitHub Desktop that manages this nicely most of the time.

I copied all but fixed required package loading from project file paths e335493 verbatim.

what was wrong with it?

I try to understand, how a nodes package can be specified

I would keep it just as it was after my commit plus looking in ryven_dir_path(), that is: node packages can be specified on the command line

by full path (of the directory, without nodes.py)
by name if they are

an example package (as it did before) or otherwise
in the default location (ryven_dir_path(),  ~/.ryven/nodes/ on linux)

implicitly by loading a project

if the path ( dir field) is not valid it's ignored i.e. has to be specified manually
if a package with the same name is specified manually (first two options) it is preferred no matter if dir holds a valid path or not

Should it also look in ryven_dir_path()?

Yes, I think this was not included before

I wonder if searching in Ryven's directories should be restricted to names without any separators in it to avoid (unlikely) unexpected results?

I don't think so, isn't it common to simply put it in quotes in that case?

How should nodes packages specified in project files be handled? These have a name and a dir, which is a full path. Here the behaviour is not so clear cut. The graphical interface StartupDialog looks for packages in the specified path; if they cannot be found, the user is presented with the choice to automatically look for these packages in Ryven's directories based on the name. That is very convenient, but how should that be handled on the command line?

No, I think from the command line there should not be completely automatic search, as when using command-line arguments you probably want slightly more manual control, I think automatic search as the SelectPackages_Dialog does might be rather confusing there.

One way is to not handle it at all;

that's what sounds most obvious to me.

BTW: At the moment my code does not distinguish between the name and the dir attribute in the project's file. Should it try to load the package by the file name and if failing use the name attribute to locate the package in Ryven's directories? Or – alternatively – use the last part of dir as package name?

The path and the name filed in the required packages list in the project file is currently ambiguous I think. However automatic import based on the path specified in the project file should be done only with that exact path imo. Because if that path isn't valid, the project file originates from a different system, so the user should make sure that it does not load any wrong packages and specify them manually (once), just as the GUI dialog requests.

I think automatic search as the SelectPackages_Dialog does might be rather confusing there.

while it's not top priority, I think we can add something like -n --auto for the auto search feature later, but then I'd pull it out of the dialog, see SelectPackages_Dialog.auto_import()
btw. I think I spotted a bug there
self.auto_import(packages_dir=abs_path_from_ryven_dir('packages'))

should be
self.auto_import(packages_dir=join(ryven_dir_path(), 'nodes'))

I'll fix this later

git checkout argparse
git pull origin

I was of the impression, that's exactly what I asked Git Cola to do …
Thanks for your explanation. Your code is working, but I already had planed some changes, because I do not like for a function to exit a program (that sys.exit() in prepare_nodes()). I believe that the calling method should decide what to do, so I planed to return both the packages found and those, which were not found. That offers some other possibilities …
I'll have a look at it in the coming days.

I wonder if searching in Ryven's directories should be restricted to names without any separators in it to avoid (unlikely) unexpected results?

I don't think so, isn't it common to simply put it in quotes in that case?

I was not thinking about spaces and other ‘awkwardnesses’, but what will happen, if algebra/solvers is specified and there is no local package available. Should Ryven look under ryven_dir_path() for the file algebra/solvers/nodes.py?

... I do not like for a function to exit a program (that sys.exit() in prepare_nodes()). I believe that the calling method should decide what to do, so I planed to return both the packages found and those, which were not found. That offers some other possibilities …

I think it's Qt convention to deploy the event loop in sys.exit(), not sure about the reasons for this.

Should Ryven look under ryven_dir_path() for the file algebra/solvers/nodes.py?

Ahh, sorry misunderstood that. I would not make a distinction here, and just handle it the way I described in the above post, i.e. first look for system path /algebra/solvers/nodes.py => doesn't exist? => look for a default package .../example_nodes/algebra/solvers.py => doesn't exist? => look for user package ryven_dir_path()/nodes/algebra/solvers.py

On Llinux it is the convention to look first in the user directory (ryven_dir_path()/nodes/) and then in the system wide directories (example_nodes/), so that the user's files take precedence over the system-wide ones. Is this handled differently in Windows?
Re sys.exit(): That is probably just a personal preference, but if the function is reused in a different setting, I like to give a chance to the calling program to recover. I'll have a look, which implication that has and report back.

On Linux it is the convention to look first in the user directory (ryven_dir_path()/nodes/) and then in the system wide directories (example_nodes/), so that the user's files take precedence over the system-wide ones.

oh, I didn't know there are conventions about this. But I don't see a problem in switching precedence of example_nodes/ and ryven_dir_path()/.

Is this handled differently in Windows?

On Windows there are only relative and absolute paths, and they are distinguishable because absolute paths start with a partition (C:/). But AFAIK there are less clear conventions for where to put things

This order even makes some sense: It is needed, so that you can actually override system settings with your own preferences. By using system paths first, this would not be possible.
Yes, I noticed this on Window: I can never find a (configuration) file I am looking for! :rofl:

I'd like to rebase this PR to dev branch (instead of main) if you're fine with that? dev only has 8b4a63cf947d49149f5715ea35dbdce02b9b2e22 ahead which you already included here, so there should be no conflicts

By all means, it your repository!
Can you please tell me, what I have to do to get it updated (and not loosing the work I've put in in the meantime).
BTW I thought, I had branched off dev, but I have failed again to master git …

You did everything right, as you made new branches for your changes which ensures that my rebasing will not cause confusion, just changes the target branch on my side.
Just push your changes to origin (your argparse branch on github), the commit(s) should then appear in this PR, and I will rebase

My changes are not yet ‘pushable’. Please rebase the argparse branch, I'll get my local changes into the rebased branch.

Sorry to bother you again with git-related stuff. I now got the updated repository. My git-graph now shows, that the argparse branch was merged to master. Is that correct? Where should I add my changes now? I guess it is still the argparse branch, but I am not sure.

not sure which master branch this is referring to (mine or yours), but in any case you should just push to your argparse branch

I hope I made everything right re git.
Re the flow themes: There is a commit in ryvencore-qt (also branch argparse), which makes the finding of the flow theme case insensitive. I seem to remember, that you made a similar change, but I cannot find it.
Re the loading nodes packages: IMHO this makes the loading a bit less messy. As a start, it is now possible to use -n pkg1 pkg2 as well as -n pkg1 -n pkg2 at the command line. Secondly process_nodes works with set()s instead of list to automatically remove duplicates and now returns one set of packages found and one set of packages not found. The main program can now decide what to do.
This also would make it possible to rework the StartupDialog, which IMHO should recreate the fundamental functionality of the command line argument. I think along the line, that it should honour all arguments given on the command line, but offer to change them. At the moment I can think of the following functionality:

Load/change a project file / example. If none is given, a new project is created.
Load/change nodes packages.
Change window theme.
Change flow theme.

What do you think?

This is roughly what I have in mind for the StartupDialog:

First of all, this looks great. This command-line interface is getting really efficient and intuitive, I love it.

[...] StartupDialog [...] should recreate the fundamental functionality of the command line argument.

why exactly? I mean the interfaces are quite different, so forcing them to offer the exact same options might be ineffective, no?

* Load/change a project file / example. If none is given, a new project is created.
[...]
* Change window theme.

That's what the startup dialog implements so far.

* Load/change nodes packages.

That's what the SelectNodePackages_Dialog is for. Additional node packages can be imported through the editor (which also has a shortcut for this). Providing the option to load node packages in both ways could cause confusion I think, it sort of implies that the two ways are different in the result, which is not the case.

* Change flow theme.

That is also done quickly through the editor itself. But if you really want this, I don't think it would hurt anyone.

Regarding the StartupDialog: My thinking is, that the commandline-affine person can give the options on the command line. The programmer can call the run() function with keyword arguments to get the same effects.
This leaves the command-line hater. To cater for him/her, there is the StartupDialog: Ryven started with the --show-dialog (or -s) option would give him/her the same (or at least similar) experience.
In my current implementation it is also possible to mix the command line / function keyword arguments and the --show-dialog option: The StartupDialog will adapt to the commandline arguments, e.g. if the --flow-theme is given on the command line, the drop-down box with the flow themes will have this theme preselected.
The SelectNodePackages_Dialog will be incorporated next to the Nodes packages: label. (That's what I am doing at the moment.)

In my current implementation it is also possible to mix the command line / function keyword arguments and the --show-dialog option

I really don't think we need this...
I see your intuition to make the interfaces somewhat equivalent, but still: why? As I wrote above, I'm concerned that ambiguity could cause confusion. It destroys the clean initialization process. The dialogs currently require the exact amount of information they need to initialize a session, nothing more, nothing less. I feel kinda reluctant mixing this with lots of other options which you can just select in the editor once the session is running, with no additional effort.

The SelectNodePackages_Dialog will be incorporated next to the Nodes packages: label. (That's what I am doing at the moment.)

Is there a reason to merge the dialogs, other than for easing the compatibility with command-line arguments?

In my current implementation it is also possible to mix the command line / function keyword arguments and the --show-dialog option

I really don't think we need this...

This does not take much effort (one line per setting) and there is an idea behind it: Let's say, you always want to start Ryven with the same configuration (window theme, flow theme, nodes packages) but you want to specify the project not on the command line. With my approach you can make a shortcut with this contents:
ryven --window-theme=light --flow-theme="colorful light" --nodes=std --show-dialog

Using my idea, this would open the StartupDialog with the options pre-filled in and you can select the project file with the help of the GUI.
After having selected a project file, you could even reconsider your choice and select a different one (because you have clicked on the wrong one in the first place). With the current implementation Ryven is started immediately after having selected the project file.
You will see, which nodes packages are required and can select to override it with your personal nodes package.
You can also change the window theme after having selected a file.
These were some of the considerations which led me to this approach.
BTW you could also say, that setting the --debug or --flow-theme options on the command line are not necessary, because this can be done directly in Ryven …
(This reminds me to include the --debug option into the StartupDialog …)

I believe one of the important features (if not the most important one!) of such a StartupDialog is the possibility to override any nodes package with your own choice (as it can be done on the command line).
This is how it currently looks:

Whenever you create a new project/load an existing one or an example, the two columns  and Imported:``Missing: are updated. If any of the nodes packages in those two columns are overridden with an package imported by the user, this is marked as can be seen with the linalg package above.
You can populate the Manually imported: column with the help of the Auto import and Import buttons.
The list with the manually imported packages can also be manipulated with the Remove and Clear buttons.
The items in all three lists have tooltips showing the path to the package.
The four buttons adapt to the situations and the Ok button only becomes available, if there are no missing packages.

Up to here, everything is without the major changes in the StartupDialog proposed above …

The last commit is self-contained and can be removed if not liked (but then I have the gut feeling, the pre-commit logic for --show-dialog needs to be adapted …)

cannot load projects through GUI anymore
$ ryven -s
Traceback (most recent call last):
  File "/home/leon/.local/bin/ryven", line 8, in <module>
    sys.exit(run_ryven())
  File "/home/leon/.local/lib/python3.9/site-packages/ryven/main/Ryven.py", line 465, in run
    args.project = sw.file_name
AttributeError: 'StartupDialog' object has no attribute 'file_name'

cannot load projects through GUI anymore, at state of 15cd69d

No, you can't, because of:

(but then I have the gut feeling, the pre-commit logic for --show-dialog needs to be adapted …)

Sorry, if my comment

The last commit is self-contained and can be removed if not liked

was misleading. Since I worked in parallel on the run() function and the new StartupDialog I did not want to wast time and effort, to get the old StartupDialog to work. Everything apart of -s should work with 15cd69d.

You are right. An oversight on my side, but I am really surprised, that it worked for you on the command line …

looking good now, I will make a few proposals once I can do more detailed testing soon

If you could also have a look at the FIXME:s, I would be grateful!

Loading user project files in ryven_dir_path()/.ryven/saves/ only works by giving the actual path. I suggest that one can also spcify e.g. test/test.json (both should work imo) for loading ryven_dir_path()/.ryven/saves/test.json, and mydir/test/mydir/test.json for loading ryven_dir_path()/.ryven/saves/mydir/test.json, just as it is with node packages already.

I'm sure, that can be done.

* Proposal to rename `debug` option (also in dialog) to `default-output` or something like that, because it only regards where output is directed and does not necessarily have to do anything with debugging.

I don't mind to rename it. The idea behind naming it --debug was, that it prints messages, which help in debugging the code (this is pretty common, at least, in the Linux world). Would --verbose|-v be a more appropriate parameter name?

* `--performance pretty` has no effect yet (I think I will have to look at that in mainwindow)

When I checked, it switched the checkbox in the mainwindow's menu … (Honestly, I never saw a difference with this enabled or disabled.)

* same for `--no-animations`
* same for `--info-messages`

Both switch the appropriate mainmenu's menu. Doesn't that switch these options on/off?

* Proposal to show dialog by default and implement option to _disable_ it. New users unfamiliar with command-line options should see it by simply running `ryven`, imo.

I beg to differ here. The reason, why you have command line arguments, is, that you do not want to hop through a GUI by default.
If it is important to show this GUI as a default in the first place, then I would suggest to add the possibility to open a project file directly from the mainwindow … This is quite a design decision which might confuse the new user.
Apart from this deliberate design decision to not include an Open project … menu item, there are other considerations:

It is always possible to print a message on the terminal informing the user, that there is a --show-dialog|-s option available.
If the user always wants to start with the StartupDialog, it is possible to write a configuration file and add show-dialog to it. We could also implement a default configuration file in ryven_dir_path()/.ryven/ryven.cfg, which is always read (mind you, that would be a good idea anyway).
If Ryven is opened from the start-up menu (or a link on the desktop), this should include the --show-dialog option. (I do not know, if that's possible on Windows, it is certainly easy on a Linux system.)

Anyway, I would very much prefer to add an Open project ... menu item, if started from the command line. If that is not wished for, because the main window is embedded into another application, this could be switched off with the help of a parameter (or switched on, if started from the command line).
I believe, that is the cleanest option which would not confuse any (new) user, because it behaves as expected.
What do you think?
BTW such an Open project ... menu item would gain enormously, if the functionality of loading/overriding nodes packages in incorporated in the open file dialog.

* when I override automatic import from project file manually, even though I select the exact same file, the nodes are not imported and the editor crashes

Strange, it seems to work for me. Can you please tell me step by step, what you do to crash the editor, so that I can have a look.

++ for adding plain theme

:blush:

Would --verbose|-v be a more appropriate parameter name?

Also doesn't sound quite right to me, because it doesn't cause any more output, you get exactly the same output, just in a different location. Maybe --stdout|-o?

When I checked, it switched the checkbox in the mainwindow's menu … (Honestly, I never saw a difference with this enabled or disabled.)
[...]
Both switch the appropriate mainmenu's menu. Doesn't that switch these options on/off?

It does slightly change the way nodes and connections are drawn when you change it in mainwindow. Specifying via command line currently only changes the menu in mainwindow but the change doesn't take effect. But I think I can fix this easily.

[...]
What do you think?

ok, good points. Then I think we should make sure not only the command ryven (corresponding to run()) is registered [unregistered] during install [uninstall], but it should also automatically register [deregister] another function run_as_app() (calling run(show_dialog=True)) as application in the system, so you can e.g. search Ryven in your applications or start menu. There is probably an OS-independent pip feature for this, but I'm not aware of it.

Strange, it seems to work for me.

crate new project
import std examples nodes package
place some std node in the scene
save project
close Ryven
open Ryven dialog
import the project

here it should automatically import std from the path in the project file

click IMPORT and manually select the same std example nodes package again

this will override the automatic import

click OK

then it crashes. does it work for you?

Would --verbose|-v be a more appropriate parameter name?

Also doesn't sound quite right to me, because it doesn't cause any more output, you get exactly the same output, just in a different location. Maybe --stdout|-o?

It's up to you … For me (with a Linux background), it is more verbose, because I always expect that --debug or --verbose puts out more strings at the terminal and that's exactly what this parameter does!

When I checked, it switched the checkbox in the mainwindow's menu … (Honestly, I never saw a difference with this enabled or disabled.)
[...]
Both switch the appropriate mainmenu's menu. Doesn't that switch these options on/off?

It does slightly change the way nodes and connections are drawn when you change it in mainwindow. Specifying via command line currently only changes the menu in mainwindow but the change doesn't take effect. But I think I can fix this easily.

 That should not be necessary, if they are proper radiobuttons.

[...]
What do you think?

ok, good points. Then I think we should make sure not only the command ryven (corresponding to run()) is registered [unregistered] during install [uninstall], but it should also automatically register [deregister] another function run_as_app() (calling run(show_dialog=True)) as application in the system, so you can e.g. search Ryven in your applications or start menu. There is probably an OS-independent pip feature for this, but I'm not aware of it.

I can only say, how that works on a Linux system:

The ryven command is ‘registered’ as a regular command (that is, an executable is put in a special location), so that it can be started from the terminal.
For the desktop environment, a ryven.desktop file is written, which would start ryven with the --show-dialog parameter. This file also contains a link to the icon, the name it should show in the start menu and some (short) description, what the program does,

That's all there is to do on a Linux system.
My preferred way would be to add a File open menu item into the File menu if ryven is run as a stand-alone application. There might be other menu items, which should only be available, if run stand-alone. And if it is run embedded, the developer might want to restrict the loading of additional packages or even the saving of the project. Maybe s/he even wants to add additional menu items? Could that menu system be made more flexible? (E.g. I know from other apps, which can be run stand-alone and embedded, that they have a way to add menu options, but all this might be implemented as a next step …)

Strange, it seems to work for me.

* crate new project

* import `std` examples nodes package

* place some `std` node in the scene

* save project

* close Ryven

* open Ryven dialog

* import the project

  * here it should automatically import `std` from the path in the project file

* click IMPORT and manually select the same `std` example nodes package again

  * this will override the automatic import

* click OK

then it crashes. does it work for you?

Thanks for the step-by-step description. I have never tested it before, but I can confirm, that it does not work for me.
First I thought, the reason is that the Ryven import system adds the nodes twice (ryvencore/Session.py uses a list with append() instead of a set), so changing it to sets should help:

ryvencore/Session.py, L29: Replace [] with set()
ryvencore/Session.py, L30: Replace [] with set()
ryvencore/Session.py, L48: Replace .append with .add
ryvencore/Flow.py, L85: Replace self.session.nodes + self.session.invisible_nodes with `self.session.nodes.union(self.session.invisible_nodes)

Unfortunately it still crashed (it might still be a good idea to use a set() here!)
I then noticed the following behaviour:

The manually imported nodes package is regularly imported.
When the nodes package is imported a second time (because it was specified by the project file), Python takes care to not import it twice.
But the session tries to register these nodes again!

This is – what I believe – what happens in ryvencore.Session.register_node(), which is called with each node of a package:

node_class.build_identifier() (L46) builds a new identifier string, in our case it is std.std.NODE for the second import instead of the original std.NODE. But build_identifier() not only builds this new identifier, it also stores it in the node class. We now have effectively changed the identifier of the class, which was imported first. NODE cannot be found with std.NODE anymore!!
self.nodes.add(node_class) (L48) does not do anything, because this class is already in the set.

There is a side-effect, which is probably not wished for: If those two nodes packages have the same package name, but different nodes, the additional nodes of the second package are imported and made available! (I wonder, if the same happens, if two packages with the same names are imported interactively?)
At the moment I can see two possible solutions:

The run() method removes the obsolete nodes packages.
There is a mechanism in the Ryven's import mechanism, which keeps tracks of imported packages and stops subsequent tries to import a package with the same name (which I would prefer, because this would also solve the problem described above)

For me (with a Linux background), it is more verbose, because I always expect that --debug or --verbose puts out more strings at the terminal and that's exactly what this parameter does!

well if that's the convention (which I don't know) then easy peasy we do --verbose|-v

I was also taken by surprise, that I had to switch the option on and the corresponding anti-option off. That should not be necessary, if they are proper radiobuttons.

It shouldn't, I will look at that once I find time to go through all the code. For now you can just leave it, your run() function obviously gives the correct instructions to mainwindow, there is just some bad ordering in gui and session initialization going on.

That's all there is to do on a Linux system.

ngl, this is beautiful. but I'd prefer outsourcing this to pip so it manages version-based changes and automatic removal etc. correctly

My preferred way would be to add a File open menu item into the File menu if ryven is run as a stand-alone application. There might be other menu items, which should only be available, if run stand-alone. And if it is run embedded, the developer might want to restrict the loading of additional packages or even the saving of the project. Maybe s/he even wants to add additional menu items? Could that menu system be made more flexible? (E.g. I know from other apps, which can be run stand-alone and embedded, that they have a way to add menu options, but all this might be implemented as a next step …)

I want to avoid project load functionality from within the editor. Not because it's impossible but it potentially introduces a lot of undefined behavior, depending on how sophisticated and stateful your node packages are (been there, done that).
For the menu: I think this should be discussed elsewhere (but it's nonetheless important). For now, I would avoid this completely.

NODE cannot be found with std.NODE anymore!!

well spotted, that could be.

If those two nodes packages have the same package name, but different nodes, the additional nodes of the second package are imported and made available! (I wonder, if the same happens, if two packages with the same names are imported interactively?)

The whole node-package concept is defined by Ryven, so ryvencore is not aware of it and only knows nodes. Currently, multiple node packages with the same name are strictly forbidden. I think for now the advantages of pushing the user to name his node packages uniquely and precisely as well as the simplicity of this implementation far outweigh this limitation.

At the moment I can see two possible solutions: [...]

I could swear that I added a duplicate import check a few months ago in Ryven somewhere, but I couldn't find it now. In any case, this should definitely be handled by Ryven, and I'd go for option 2 as well.
E.g. we can statically store imported package names in NodesPackage and add a non-static is_duplicate() method, and check this in ryven.utils.import_nodes_package. Something like that.

So, --verbose it will be …
Thanks for looking into the switching of those settings. I guess, as soon as this is sorted, just one call (either editor.ac_anims_active.setChecked() or editor.ac_anims_inactive.setChecked()) should be sufficient?
Do you want to do the check for already registered packages yourself or should I have a look?

Do you want to do the check for already registered packages yourself or should I have a look?

done.
Also, I suggest renaming auto import feature to auto discover, because auto-import is rather the automatic import from paths given in a project file, while the current auto-import button is trying to discover packages in some specific places. What do you think?

Thanks for changing the debugs to verbose and for prohibiting multiple package registration.
Auto discover sound so much better! To keep the button short, maybe just Discover or even Find is enough? Please go ahead and change it to your liking.
Is there anything else which should be taken care of at the moment?

Loading user project files in ryven_dir_path()/.ryven/saves/ only works by giving the actual path. I suggest that one can also spcify e.g. test/test.json (both should work imo) for loading ryven_dir_path()/.ryven/saves/test.json, and mydir/test/mydir/test.json for loading ryven_dir_path()/.ryven/saves/mydir/test.json, just as it is with node packages already.

I'm sure, that can be done.

I'd be grateful if you could add this

I'll have a look!

May I suggest to move --version and --verbose down

1110ca27436f38497241a5c2f6bd0c37e101b09b

That problem exists as long as command line arguments exists!
When implemented this, I started with nargs='+', action='extend' (chaining), switched to nargs=1, action='append' and then back to nargs='+', action='extend', because I was too lazy to specify a -n in front of every nodes package. I never run into any problem – because probably I am used to this and automatically put a -- between the nodes package and project file:
ryven -n std -- ~/.ryven/saves/test5.json

We could now mention this in the help documentation or switch back to nargs=1, action='append' (or action='append', because nargs=1 is the default) ...
If we decide to keep the nargs='+', action='extend', I will change the help text to mention the --, because I have to rewrite it anyway for loading project files from ryvend_dir_path() (https://github.com/leon-thomm/Ryven/pull/96#issuecomment-1076403031)

ryven -n std -- ~/.ryven/saves/test5.json

hm true, that works too. I don't know, I would intuitively prefer requiring -n in front of every nodes package (I mean writing -n is super quick, and it makes this quite a bit more intuitive). Also, usually, you only use very few node packages at once...

The run(nodes=['std', 'linalg']) compatibility is a good point... considering this, if you add an explicit note to the help text of -n as well as project then I also support chaining.

No problem, I'll to that.
Adding such explanations are A Good Idea™ anyway, because it might not be so obvious to anybody not familiar with (Linux) command line utilities!

quick question: I am trying to add a tiny set of command-line arguments to another module in a similar fashion as you did it in Ryven.py. However, when I specify a path the Linux way with ~/.ryven/..., pathlib.Path(...).exists() always returns False when I use ~. Then I looked at Ryven.py, but there the path argument, even when given with ~ is always automatically translated into the full path, but I don't quite understand where that happens exactly.

I just checked and ~ is already expanded to /home/sphh/ before it reaches the argument parser. That means that sys.argv has already the expanded path in it! (At least on Linux.)
If you generate this path inside your program, I believe you have to call pathlib.Path('~/.ryven').expanduser(), but I do not know, if that will work on Windows. (And yes, you could probably argue, that this should be done automatically when calling pathlib.Path(...) …)

yeah, seems like it. Weirdly, I cannot find any reason for this, or why it doesn't work in my other module. Looks like a side effect of something. In any case: is there a useful reason why pathlib is not able to expand ~ automatically? In my opinion, this is really bad and not intuitive, and could lead to a lot of nasty bugs. In fact, we already got one:
    run(use_sysargs=False, project='~/.ryven/saves/test5.json')

already causes an unreasonable crash, because it cannot find the path.

to be fair, os seems to handle it somewhat similarly. I think ~ is usually expanded by the shell. Then I think we can assume non-~ paths when given programmatically. However, for the command line arguments, as sometimes it does not seem to get expanded correctly, we should add expanduser to the paths given, e.g. in Ryven.py L305
            project = utils.find_project(os.path.expanduser(args.project))

instead of
            project = utils.find_project(args.project)

and
        args.nodes, nodes_not_found, _ = utils.process_nodes_packages(
            set([os.path.expanduser(pkg_path) for pkg_path in args.nodes])
        )

instead of
        args.nodes, nodes_not_found, _ = utils.process_nodes_packages(args.nodes)

I asked Dr. Google and I think with

I think ~ is usually expanded by the shell.

you might be on the right track! https://stackoverflow.com/questions/3488603/how-do-i-use-tilde-in-the-context-of-paths#answer-3489177 gives an explanation which actually does make a lot of sense.

Then I think we can assume non-~ paths when given programmatically.

I think, this is a requirement, because ~ can be part of a pathname (wasn't Windows using the tilde for shortening long file names to 8.3 file names?).

However, for the command line arguments, as sometimes it does not seem to get expanded correctly,

When would that happen? I'm of the impression, that ~ standing for the user's home directory should never reach the argument parser, because it is expanded by the shell.

When would that happen? I'm of the impression, that ~ standing for the user's home directory should never reach the argument parser, because it is expanded by the shell.

you would think so. And I just tried again for my other module as well and now it works... :roll_eyes: I don't even know how I could influence that myself as it should be handled completely by the shell. Anyway, I think we can assume that the shell correctly expands the path then...

(wasn't Windows using the tilde for shortening long file names to 8.3 file names?).

oh, I don't know, but that totally sounds like Windows. os.path.expanduser() seems to cover that case, it only expands if ~ is the first symbol, I think. But still, I think we should not expand any paths manually if that's not how it's supposed to be done.
So, right now everything works for me as it is.

Alrighty, anything else you would like to add to this PR?

That's what I also want to know!! Probably, because of my many mistakes …

anything else you would like to add to this PR?

No, thank you, no more changes! :roll_eyes:

Is that on purpose, that you drop all unknown arguments in line 102? This is quite unusual …

Yeah, kind of. I wouldn't know what else to do with them. I don't really see where they could be intended to go. RyvenConsole really does nothing other than deploying the session without GUI dependencies and spinning up a REPL. Is there something more appropriate I could do there?

because of my many mistakes …

probably because you have the skill for perfection :)
Alright, I'm also done for now, I think. I will merge to dev, do some more casual testing tomorrow or something, slightly rework the README, maybe add something to the website, and publish v3.2.0 asap :tada:
Thanks a lot for spending so much energy contributing to this project :fist_oncoming:

Is that on purpose, that you drop all unknown arguments in line 102? This is quite unusual …

Yeah, kind of. I wouldn't know what else to do with them. I don't really see where they could be intended to go. RyvenConsole really does nothing other than deploying the session without GUI dependencies and spinning up a REPL. Is there something more appropriate I could do there?

Traditionally programs fail, whenever they get unknown command line arguments.
I just used this approach, so that command line arguments are made available to Qt without hardcoding these arguments. But I admit, that I don't like this solution and would love ryven to fail, if it gets a command line argument unknown both to ryven itself and to Qt. Any ideas?

would love ryven to fail, if it gets a command line argument unknown both to ryven itself and to Qt. Any ideas?

True, the Qt application seems to ignore unknown arguments, at least I can give random ones and it just continues. Hardcoding available Qt app arguments renders the feature of automatically passing remaining arguments useless. But if ignoring them is the way Qt does it, is there a problem with accepting that? Ultimately available arguments can probably differ between operating systems, so it does sound clean to me to just give them to Qt and not care about them anymore

There are a couple of ‘problems’ I have with this approach:

I would like to know, if I have given a command line parameter, which is wrong; so the program should tell me so (and show a short help message).
I would like to know, which parameters are available for Qt; Gtk has something like --help-gtk and --help-gapplication, which would tell me all possible parameters. (I could not find an equivalent for Qt.)

Issue 1. could be dealt with with calling app.arguments() after initialization of the QApplication. If it returns an empty list, all arguments were consumed by the application, otherwise an error (or warning) could be printed – even indicating the wrong command line arguments –, but it leaves the user in the dark, which arguments are actually possible (issue 2.)
I do not know, if we can do anything about 2.?
When investigating I stumbled upon two other problems:

Some of Qt's arguments interfere with our own definitions: https://doc.qt.io/qt-5.15/qapplication.html#QApplication gives the general arguments (there are more on Linux). For example if you start ryven with ryven -style=cde this is interpreted by argparse as -s and the startup dialog is shown. For me this looks like a shortcoming of Qt, because it uses long arguments with just one - (instead of --, the standard).
Starting ryven with ryven -style cde would trick argparse into using cde as a project file name.

Issue 2. could be dealt with by adding some documentation to the help text about Qt arguments requiring a =.
Issue 1. could be solved by requiring Qt arguments to have double hyphens – and subsequentially replacing them with a single one. This would break compatibility with Qt standards.
Or we could make it a requirement to prepend Qt arguments with a -q, such as ryven -q style=cde -q bg=black.
Or we could decide that it is not worth the hassle and drop support for Qt arguments altogether (I just looked at spyder, which does not seem to support them).
Oh well, there is even another possible solution: Initiate the QApplication with all arguments and only pass those not consumed to argparse. But this would require to import Qt just for simple tasks as displaying the help or the version (and a user without Qt installed cannot get to the help information) All this could be dealt with a three step parsing:

Call argparse to deal with --help and --version
Initiate QApplication, passing all arguments and capture all non-consumed arguments
Parse those remaining arguments to argparse.

Mind you, this might be the cleanest way to deal with those Qt quirks!
BTW this reminds me: If Qt/PySide is missing, the user should get told about it and not left with a undecipherable traceback – which would be easy with the three step approach …
What do you think?

I decided to deal with the Qt arguments as outlined at the end of my comment above. It's a bit circuitous to work around those Qt quirks, but it seems to work.

I'm having some really weird issues since I pulled the latest changes, cannot even fully install it anymore even though everything seems correct. Will take a closer look once I got that sorted out.
What are the main reasons to provide Qt arguments in the first place? Since we made it conveniently integrable, we might also just enable qt arguments programmatically, via function arguments to run(). This would make this a lot less messy, I think.

I'm having some really weird issues since I pulled the latest changes, cannot even fully install it anymore even though everything seems correct. Will take a closer look once I got that sorted out.

Is this because of https://github.com/leon-thomm/Ryven/pull/96/commits/6ad3cea923897206b9185511d74b5f1adca2de21?

What are the main reasons to provide Qt arguments in the first place? Since we made it conveniently integrable, we might also just enable qt arguments programmatically, via function arguments to run(). This would make this a lot less messy, I think.

Yes it's messy because of Qt not following standard ways to specify command line arguments (at least, that's how I see it). Some developers have decided not to support them at all, e.g. Spyder (but they have implemented some features in their preferences, which are saved in a configuration file).
I have no idea, which arguments are actually supported (because I do not know how to access Qt's help), but for example this command line changes the size of the window (on Linux):
ryven -geometry 100x100

or this positions the window in the top left corner
ryven -geometry +0+0

Here are two ways, to change the (plain) look of Ryven:
ryven -style Windows -w plain
ryven -style Fusion -w plain

All of this is Qt's functionality …

Is this because of 6ad3cea?

I don't know, it doesn't make a lot of sense to me tbh. I committed these changes already in ef6ab2eabac7cd23649939afaf62349c3d4acd3a and I cannot see that anything removed them. So now it looks like it got committed twice. And the problem must come from somewhere else, maybe this just broke something somewhere and we'll have a hard time finding it. But if you can install the current state of this branch like this
pip uninstall ryven --yes
pip install .

and it runs without issues, then there's something wrong with my setup.

All of this is Qt's functionality …

Yep, I see that, but I don't yet see substantial use cases for this outside of embedding. And there it's done via function arguments anyway. Seems rather like an edge case to me that someone wants to generally reconfigure this and is not willing to just write his own tiny wrapper for run(). But I also never used Qt command-line arguments for applications.

pip install

I'll try it.
Well, setting the position and size was standard and often used in good'ol'times. I don't know, how often this is used nowadays.
I do not see a way, to set Ryven to Windows style a different way.
I would love to know, what else is lurking behind the Qt command line arguments, but I could not find it on the net. No wonder, why nobody uses them!
With these being esoteric (and having a non-standard format), I have no hard time to drop the support. So sorry, Qt!!
But I would love to keep the -geometry argument (and rename it to --geometry). Would this be a way forward?

I'll try it.

my computer just needed a reboot (pain.)

Would this be a way forward?

yes, I think that sounds reasonable to me, and I have nothing against a geometry argument.

analogously to b0c735b8c12385800382ba9d183ce7f73cd6944a we should add loading config files from ryven_dir_path(), e.g.
ryven @conf1.cfg

should load the config file ryven_dir_path()/ryven1.cfg.

my computer just needed a reboot (pain.)

Ah, The Windows Solution™ :roll_eyes:

Would this be a way forward?

yes, I think that sounds reasonable to me, and I have nothing against a geometry argument.

Ok, I'll add it to the argparser and pass it to QApplication …

analogously to b0c735b we should add loading config files from ryven_dir_path(), e.g.
ryven @conf1.cfg

should load the config file ryven_dir_path()/conf1.cfg.

Aha, we are back, were we started :grin: It's always a good idea to have a default configuration file!
May I suggest to call the file ryven.cfg? Then you already know from the file name (even if it ended in a different place by accident), which program it belongs to.

Ah, The Windows Solution™ :roll_eyes:

It's ubuntu KDE! :<

May I suggest to call the file ryven.cfg? Then you already know from the file name (even if it ended in a different place by accident), which program it belongs to.

Sure, sounds good.

Looking good! Should we add automatic creation of the default config file during installation to setup.py?

Yes, why not? Perhaps a reference to ryven --help and the last section about configuration files would not go amiss?
I just notices another problem: Some arguments to the --flow-theme can contain spaces (pure dark, colorful dark and their light equivalents). This interferes with putting nodes: std linalg into the configuration file – and also requires the user to enclose such themes in quotation marks. Would it be possible to replace the space in the theme's name with a hyphen - or underscore _ (at least for the command line parsing)?

Would it be possible to replace the space in the theme's name with a hyphen - or underscore _ (at least for the command line parsing)?

Certainly possible but maybe a kind of annoying restriction. How does this interfere with nodes in config files? If your nodes package contains spaces (which is obviously undesirable but shouldn't be a restriction) you also need to put it in quotation marks, right?

Needing to put quotation marks around arguments is prone to errors (and very annoying) – especially if you do not need it all the time …
I run into troubles, when I tried to specify more than one nodes package in the configuration file, e.g. nodes: std linalg tries to load a nodes package at std linalg, which obviously does not exist. This can be dealt with by only giving one package in one line as you have done in https://github.com/leon-thomm/Ryven/pull/96#pullrequestreview-922441403, but this does not follow the command line syntax verbatim.
When I try to split the nodes package names at spaces in convert_arg_line_to_args(), it would also split flow theme names with spaces in it. The easiest way around this is to have flow theme names (and nodes package name) without spaces. Alternatively it might be possible with a bit of parsing to only split at white spaces, which are not inside quotation marks (I've not looked into this yet).

Hmm, these workarounds sound quite unintuitive to me. I see yet another reason in this to not support chaining of nodes packages, as suggested in https://github.com/leon-thomm/Ryven/pull/96#issuecomment-1076491726
And in response to your earlier comment on this, that is is mainly to keep compatibility with providing nodes packages as function arguments like this:

run(nodes=['std', 'linalg'])

I think it'd we much less weird to extend the function argument capabilities to run() such that this is possible, other than starting to convert name, and when someone changes something in ryvencore-qt or creates a nodes package that has weird characters in names everything breaks down with no reasonable source to the user/developer.

For me it is unintuitive to have sometimes quotes and sometimes no quotes (and yes, I even use file names with spaces without quotes!)

(I've not looked into this yet).

This turns out to be surprisingly simple by using the standard shlex module:
import shlex
values = shlex.split('std linalg "my own package"')

or regular expressions (re is already used by argparse, so it does not require a new module to be imported).
Hence we can keep the spaces in (flow themes and nodes packages) names, but then require the user to quote them, if they contain spaces!

Isn't that how it's done already? ryven -n "pkg with spaces" std will import the node packages std and pkg with spaces, I just tested.

Let's have a look at --flow-theme:

I can specify it at the command line as --flow-theme "colorful light", --flow-theme="colorful light" or -f "colorful light" (or with single instead of double quote).
I can specify it in a configuration file as flow-theme: colorful light, but not flow-theme: "colorful light" or flow-theme: 'colorful light'

This is ok and works as indented (sort of, if you disregard the discrepancy between the command line requiring the quotes but the configuration file must not have them).
Looking at loading nodes packages:

Command line: --nodes std --nodes linalg --nodes "pkg with spaces" also works.
Command line: --nodes std linalg "pkg with spaces" works.
Configuration file: nodes: std, nodes: linalg and nodes: pkg with spaces all on their own single line works.
Configuration file: nodes: std linalg "pkg with spaces" does NOT work.

It would be nice, if

the command line and configuration file would have the same syntax, that is if there is a space in the value, the value has be enclosed in quotes
specifying more than one nodes package in one line should be possible

Since you want to keep spaces in theme and nodes package names, the only way forward would be to enforce quotes around all values with spaces!

I see, it's only about the config files at this point.

Since you want to keep spaces in theme and nodes package names, the only way forward would be to enforce quotes around all values with spaces!

that does sound like the way forward to me :) I don't think anyone's gonna be mad if arguments with a single value (like --flow-theme) do not require quotes in the config file, though (because there the separation is the line break, as far as I understood it).

Unfortunately that is not so easy. convert_arg_line_to_args() does not know the action of the argument it is working on (because it just translates each line of the configuration file into a sys.args compatible list). The only way would be to make a special case for nodes:  entries! I do also like to have exactly the same syntax for the configuration file and command line arguments to remove as many surprises as possible.

sorry for the delay! I was quite busy; I'll try to respond quicker :(
I want to bring this nodes packages chaining thing up once again, cuz I still don't get it... I see the following conflict right now:

we want config files to be close to command-line arguments, for simplicity reasons
on the command line, it generally makes sense to require quotes about values with spaces
in the config file, it does not necessarily make sense (e.g. it is not intuitive that flow-theme: pure dark wouldn't work)

Now to the roots of those issues:

If we allow chaining of node packages, since they can have spaces we have to force quotes around all values with spaces in config files as well
Furthermore, allowing chaining required us to introduce --, which IMHO is much less intuitive than writing -n std -n linalg instead of -n std linalg, this really looks like a strange tradeoff choice to me.
IMHO writing in config filesnodes: std
nodes: linalg

is much cleaner than nodes: std linalg.

Assume we remove chaining, this has the following effects

It solves all the above issues; command line requires quotes, config files don't as there are only single-valued arguments
It introduces the problem you mentioned in https://github.com/leon-thomm/Ryven/pull/96#issuecomment-1076498514. However, I don't see a problem in extending the functionality of run() such that run(nodes=['std', 'linalg']) is possible, i.e. it feels way more natural to me than forcing -- syntax and weird config files.

Is there some deep Linux convention or something applied here which I don't know? Because otherwise I really don't see why we should stick to this feature. What do you think?

sorry for the delay! I was quite busy; I'll try to respond quicker :(

No problem, as long as you are ok!
The current implementation is the only one (at least the only one I can think of) to keep the syntax of all three interfaces – arguments to run(), command line arguments and configuration file – the same.
In my personal view, all these interfaces follow standard good practice:

Needing '--' to separate keyword arguments and positional arguments on the command line is standard on Linux. Even the most fundamental programs use this approach. That '--' is a standard can be seen in the fact that Python's argparse implements it ‘out of the box’.
I personally find it counter-intuitive to specify an argument multiple times for just giving a list of values. That's true for command line arguments and configuration files.
I could also argue, that ryven --flow-theme "colorful light" -n std is counter-intuitive, because I would love to write ryven --flow-theme colorful light -n std, which I would find more intuitive, but it was once decided that in computer-speak spaces separate tokens.
An alternative syntax for the configuration file is to use the equal sign: flow-theme=colorful light. If compared to flow-theme="colorful light", I would think the second looks more intuitive, doesn't it? (I added the syntax with double colon, because it looks so much nicer!)

One could also say, that the root cause is not the chaining of nodes, but of allowing spaces in argument values (flow themes, nodes)! :upside_down_face:
Having argued in favour for the current implementation, it is certainly possible to make an exception in one or the other interface!
I believe we have to keep the indented audience in mind. Let's not talk about the arguments to run(), because a programmer is used to deal with quirks all the time. So who will use the command line or a configuration file (which s/he has to write her/himself)? That is certainly a sort of power user. For the real power user and the die-hard computer gurus it does not really matter, if it is well documented. So we do not have to care for them.
Let us look at the aspiring power user: We could guess that s/he starts experimenting with the command line. Then s/he will notice, that s/he writes the same arguments everytime and will think: ‘A configuration file would be great, so that I do not have to repeat myself!’ (or s/he writes a shell skript/batch file with the arguments). With the current implementation s/he just has to copy the existing command line arguments into a file, break the line and replace the keys with their long version. The quotation marks will be preserved. Job done.
Having no quotation marks would definitely help the user to write a configuration file from scratch.
If we want to go really fancy, we could add a new command line argument, which would write the configuration file from the current command line arguments (there actually are programs providing this) – but read on.
All we can say so far, is that this does not leave us any wiser. There are advantages and disadvantages for each approach.
IMHO the absolute best approach, which avoids a lot, if not all, of these problems, is to have a configuration dialog. This would also enable a newcomer to generate a configuration file without the need to dive into its syntax. In that case it does not matter, if quotes are need or not, that can be dealt with programmatically. In my experience programs without supplying a configuration dialog is frowned upon by a lot of users, and who wants people think that about Ryven?!

Needing '--' to separate keyword arguments and positional arguments on the command line is standard on Linux. Even the most fundamental programs use this approach. That '--' is a standard can be seen in the fact that Python's argparse implements it ‘out of the box’.

That wasn't my argument though; I didn't say it's not standard, I said it's not so intuitive. One cannot guess it if he doesn't already know it. There's a lot of standard, even if you only consider Linux (or rather UNIX). Ryven is not only for Linux.

I personally find it counter-intuitive to specify an argument multiple times for just giving a list of values. That's true for command line arguments and configuration files.

I can kinda understand that for command-line arguments, but I think I have seen many cases in larger applications where this is done, and it never seemed strange to me. For config files, I disagree. Whenever you have a list of values, they are given in separate lines anyway.

I could also argue, that ryven --flow-theme "colorful light" -n std is counter-intuitive, because I would love to write ryven --flow-theme colorful light -n std, which I would find more intuitive

Then it seems like we have different intuitions :) Maybe there's a bit of my Windows experience coming into this...

[...] I would think the second looks more intuitive, doesn't it?

Again, I would not say so... how does this look more intuitive if ryven --flow-theme "colorful light" is not intuitive? From my experience I would explain it to someone like this: in config files, it is more common to not write quotes, as opposed to on the command line because the only difference to writing a long command is that the file allows newline chars, which serves as an implicit separator. (ignoring indentation)

IMHO the absolute best approach, which avoids a lot, if not all, of these problems, is to have a configuration dialog. This would also enable a newcomer to generate a configuration file without the need to dive into its syntax. In that case it does not matter, if quotes are need or not, that can be dealt with programmatically. In my experience programs without supplying a configuration dialog is frowned upon by a lot of users, and who wants people think that about Ryven?!

Point taken. But that is not a reason for me to not spend effort on the design of the config files as well. While the "die-hard computer gurus" are used to getting their hands dirty, they don't want to waste their time as well when writing it themselves, and I definitely don't want to keep those people away.
As this seems more controversial than I expected, I propose the following:

remove chaining of nodes. (and spaced values require quotes)
use a standard file format for config files, probably YAML
parse arguments to run() as command-line arguments but nodes can be given as list
as an extra: add a save configuration button to the current startup dialog

Would this be OK with you?

OMG, this opened a big can of worms!
Who is your indented audience? That's the most important question to consider!
All I can say (or repeat) is the following:

The current approach uses the Python handling of command line arguments as implemented in argparse (intuitive or not intuitive, that is up to the user).
The current approach has the very nice touch, that you can give the configuration file as a positional argument on the command line!
The current approach does not use additional external modules (YAML, JSON, etc). This restricts the possible formats, but the current format closely follows the YAML syntax key: value without complicating it (e.g. writing a list in YAML is very unintuitive, if you are not used to it)
The format of any configuration file really does not matter, if it is dealt with inside the program . This asks for a configuration dialog.
Intuitiveness depends on the user (and his/her experience, especially the stable s/he comes from – mainly Windows or Unix [where Windows users tend to not use the command line or write configuration files and aspiring gurus can deal with these quirks]). For example I personally find it very (VERY!) unintuitive, that I cannot open a project file from inside the Ryven applications …

One more thought to consider: If it is possible to add multiple node lines in the configuration file, you can have them all over the place, e.g. on line 2, 10 and 11. It is then so easy to miss that one on line 2 …
BTW if you are thus inclined you can still add multiple -n or --nodes arguments (and nodes: lines in the configuration file) – have I just invalidated my argument above?
Here is my preferred approach:

Supply a configuration dialog, which would make 99.99% of the users happy.
Then you can use whatever format (existing, YAML, JSON, XML, INI, etc) you are happy with (but see below for the choice of format).

When changing the configure file to another format, consider the following implications:

argparse treats the config files given with @ at the current position, with later arguments (command line or in configuration file) overwriting those given earlier (command line or configuration file). This is a pretty nifty feature!
Keeping this @ approach, a line-based configuration file is the only possible one, because argparse reads that file line by line and injects its return values into the command line arguments. This is not possible with the YAML format, because there a key: value pair can span multiple lines (especially for lists!) and you cannot specify the same key more than once! Here you have to use a command line argument to supply the configuration file. This is possible (and used by many other programs!), but you loose that nice feature described in item 1. (and you force the user to write the list of nodes in YAML's format for lists …)

Finally for those 0.2% of your users, who really want to use command line arguments:

Decide, if you want to force them to use a -n or --node argument for each single nodes package or require them to use a -- to separate the project file.
Deal with the consequences of this decision with regard to the configuration files.

In case, you decide to use multiple --node arguments, change the name of the arguments from --nodes to --node (but not the dest= keyword argument to .add_argument()) and remove the scanning for quotes in CustomArgumentParser.convert_arg_line_to_args(). Also update the doc string to tell the programmer, that nodes= does not follow the command line.
In case, you decide to keep the current approach, update the metavar keyword argument from metavar='PROJECT' to metavar='[--] PROJECT'.
I'll leave it to you to decide and do the changes as you think appropriate. I've already spent to much time on a simple command line argument interface …
PS: I do love the YAML format very much, but I do not consider it suitable here at all.

Thank you for these points, it does help a lot. I will finish it up. I am very grateful for this contribution, I learned quite a bit.

Thanks for keeping an open mind and not discarding any option ‘on a whim’!

I would love to have this finished and out of my mind. If you tell me exactly, what you want it in the end, I could implement it and it could be merged!

I would love to have this finished and out of my mind. If you tell me exactly, what you want it in the end, I could implement it and it could be merged!

I'm on it, just don't have much time right now, I'll maybe add a few other options which require minor internal changes, but I'm working on it.

just don't have much time right now, […] but I'm working on it.

That's why made my offer.

I'll maybe add a few other options which require minor internal changes

I am looking forward to these other options. Can you already spill the beans?

Can you already spill the beans?

adding options to disable features (like adding multiple scripts, src code preview, logs, console, nodes area, variables, and so on) for more control when embedding
adding option to add custom menu items if possible
default flow algorithm

If you'd like to help out, I would be grateful if you could implement unparse_sys_args(args) in args_parser.py() in a somewhat reliable way.

If you'd like to help out, I would be grateful if you could implement unparse_sys_args(args) in args_parser.py() in a somewhat reliable way.

No problem, see my comments in https://github.com/leon-thomm/Ryven/commit/12fdf0999ce29032980f1dc29c50deafa3194606

Seems to work for the most part. The only remaining question from my side is how we can store project files in the config. Otherwise, I think everything we wanted is implemented.

Despite having implemented it in unparse_sys_args() myself, I believe, that a configuration file is absolutely the wrong place to open a working file. It is a configuration file and not a batch/script file! That's why I have it removed now.
This is different, if a default file can be opened. But then this default file must be write protected, so that it cannot be overwritten by accident. It then acts as a template (which would be a good idea to have anyway).

Why did you remove all my changes (fixes) I did to unparse_sys_args()? Did you forget to pull? It's broken again, I'd like to jump back to my version

Simply because it was interfering with the changes I was working on as we agreed. Working on the very same problem does not even work with git …

Have you decided how to proceed with this hiccup?
All I can say is the following:

I was working on my local repository.
When I wanted to commit my changes to my local repository, I could not do it, because the upstream/original (that is your) repository had changed files I was working on.
The only way forward offered was to fetch your changes, which destroyed all my changes in that process.
I was not allowed to cherry pick changes, so I had to do them manually.

I have no idea what the root cause of that was/is.  That is absolutely not the way I want to work. I want to have my repository entirely under my control.

Sorry for the silence, I was quite sick and couldn't do a lot.

Despite having implemented it in unparse_sys_args() myself, I believe, that a configuration file is absolutely the wrong place to open a working file. It is a configuration file and not a batch/script file! That's why I have it removed now.
This is different, if a default file can be opened. But then this default file must be write protected, so that it cannot be overwritten by accident. It then acts as a template (which would be a good idea to have anyway).

Fair enough! Then I'll keep .cfg

Working on the very same problem does not even work with git … [...] I have no idea what the root cause of that was/is.

It does! With a PR your branch kinda becomes part of two repositories simultaneously, as both can merge and push to it. Git usually doesn't allow pushing if there are unpulled remote changes (it is not clear how they should be merged in case there are conflicts). If you're working on a branch which remotely changed, you can always create a temporary branch based on your PR and commit your changes there, so you don't need to worry about remote changes until you merge back into the PR branch (in case of serious conflicts, you could even create another PR there which the person that committed the remote changes (me) can review).
But no worries, everything is part of the commit history, I'll make the changes necessary. Is there anything else you want to add and didn't commit yet? Otherwise, I'll finish it up now.

I thought, I have already replied, but since I cannot find my reply here, I post it again:
Thanks @leon-thomm.

Any contributor has copyright for their code, I think there are no legal uncertainties here.

:+1:

I don't see significant use in requiring copyright waivers for this repository, though I might just lack the experience here. I know it is done and copyright waiving CLAs are standard for commercial open-source projects. As Ryven is 1. not commercial and 2. MIT licensed I think we don't need it here.

Excellent. To be honest, I was really surprised, that a copyright waiver was even considered …

I won't put all contributors into the license file - I see more problems than use for this. It's common practice to include only the project's main maintainer.

The MIT License clearly states:

Copyright © 2022 <copyright holders>

I interpret this, that all copyright holders must be mentioned there, so this should be dealt with satisfactorily.
You write, that you want to introduce an AUTHORS file (which I find a very good idea!), so I want to suggest one of the following options:

Instead of having this AUTHORS file, simply add the authors to the LICENSE file. Same effort as maintaining an AUTHORS file. (The AUTHORS file than can show something: See the LICENSE file for contributing authors.).
Keep the AUTHORS file and refer to it from the LICENSE file, but I do not see any real advantages with this approach.

Regarding more explicit acknowledgment for contributions:

I get the feeling, that there is some confusion here: That explicit acknowledgment was never requested. Under ‘explicit acknowledgment’ I understand that a contributor gets acknowledged that s/he has implemented this or that feature. That puts acknowledgements into the moral realm (compared to legally binding copyright notices). As you rightly observed, GitHub is doing a good job in that respect. Also mentioning the contributions in the release notes goes without saying.

@dumblob's comment on not confusing copyright with authorship is also really important.

If that is addressed to myself, rest assured that I perfectly well know this difference (and that I never confused it).

I don't see any reasons against an AUTHORS file, so I am open to making one that additionally lists the most significant contributors. I'll do that if nobody objects.

Please see above note about the LICENSE file.

I will remove the copyright notice with my name from the help text (which, for the record, you @sphh added, not me :upside_down_face:) and will refer to the license file instead.

Adding this was an act of courtesy – as is giving credit to all contributors and adding a proper copyright notice to the files.
Which brings me to the copyright notices in the source files: I absolutely do not see any problem with adding copyright notices to each file. au contraire, this is common practise. If the contributor wants to have his/her copyright added, is is his/her responsibility and s/he should add it to the file. If s/he does not want one, s/he simply does not add it. Simple as that.
What do you think?

@leon-thomm: I would appreciate a timely reply, because I noticed a couple of issues I would like to work on. As you can certainly understand without a resolution where everyone is happy, I do really want to invest time and effort …
Thanks.


[go_router] Duplicate GlobalKey detected in widget tree when a navigatorKey is used on a nested ShellRoute

Tested on

go_router: 5.0.1

Steps to Reproduce

Execute flutter run on the code sample
Tap on BottomNavigationBar's buttons. (More faster than page transition)

Code sample

final GlobalKey<NavigatorState> _navigatorKey =
    GlobalKey<NavigatorState>(debugLabel: 'shell');

final router = GoRouter(
  initialLocation: '/a',
  routes: [
    ShellRoute(
      builder: (context, state, child) => HomePage(child: child),
      routes: [
        ShellRoute(
          navigatorKey: _navigatorKey,
          builder: (context, state, child) => child,
          routes: [
            GoRoute(
              path: '/a',
              builder: (context, state) => const PageA(),
            )
          ],
        ),
        GoRoute(
          path: '/b',
          builder: (context, state) => const PageB(),
        )
      ],
    )
  ],
);

void main() {
  runApp(
    MaterialApp.router(
      routeInformationParser: router.routeInformationParser,
      routeInformationProvider: router.routeInformationProvider,
      routerDelegate: router.routerDelegate,
    ),
  );
}

class HomePage extends StatelessWidget {
  const HomePage({super.key, required this.child});
  final Widget child;
  @override
  Widget build(BuildContext context) {
    return Scaffold(
      bottomNavigationBar: BottomNavigationBar(
        currentIndex: GoRouter.of(context).location.startsWith('/a') ? 0 : 1,
        items: const [
          BottomNavigationBarItem(
            icon: Icon(Icons.navigate_before),
            label: 'A',
          ),
          BottomNavigationBarItem(
            icon: Icon(Icons.navigate_next),
            label: 'B',
          )
        ],
        onTap: (value) {
          if (value == 0) {
            context.go('/a');
          } else {
            context.go('/b');
          }
        },
      ),
      body: child,
    );
  }
}

class PageA extends StatelessWidget {
  const PageA({super.key});

  @override
  Widget build(BuildContext context) {
    return const Scaffold(
      backgroundColor: Colors.blue,
    );
  }
}

class PageB extends StatelessWidget {
  const PageB({super.key});

  @override
  Widget build(BuildContext context) {
    return const Scaffold(
      backgroundColor: Colors.red,
    );
  }
}

Logs

════════ Exception caught by widgets library ═══════════════════════════════════
The following assertion was thrown while finalizing the widget tree:
Multiple widgets used the same GlobalKey.

The key [LabeledGlobalKey<NavigatorState>#2b85e shell] was used by multiple widgets. The parents of those widgets were:
- Semantics(container: false, properties: SemanticsProperties, tooltip: null, renderObject: RenderSemanticsAnnotations#d87b0)
- Semantics(container: false, properties: SemanticsProperties, tooltip: null, renderObject: RenderSemanticsAnnotations#9f6a5)
A GlobalKey can only be specified on one widget at a time in the widget tree.
When the exception was thrown, this was the stack
#0      BuildOwner._debugVerifyGlobalKeyReservation.<anonymous closure>.<anonymous closure>.<anonymous closure>
package:flutter/…/widgets/framework.dart:2883
#1      _LinkedHashMapMixin.forEach (dart:collection-patch/compact_hash.dart:617:13)
#2      BuildOwner._debugVerifyGlobalKeyReservation.<anonymous closure>.<anonymous closure>
package:flutter/…/widgets/framework.dart:2827
#3      _LinkedHashMapMixin.forEach (dart:collection-patch/compact_hash.dart:617:13)
#4      BuildOwner._debugVerifyGlobalKeyReservation.<anonymous closure>
package:flutter/…/widgets/framework.dart:2822
#5      BuildOwner._debugVerifyGlobalKeyReservation
package:flutter/…/widgets/framework.dart:2891
#6      BuildOwner.finalizeTree.<anonymous closure>
package:flutter/…/widgets/framework.dart:2950
#7      BuildOwner.finalizeTree
package:flutter/…/widgets/framework.dart:3032
#8      WidgetsBinding.drawFrame
package:flutter/…/widgets/binding.dart:885
#9      RendererBinding._handlePersistentFrameCallback
package:flutter/…/rendering/binding.dart:378
#10     SchedulerBinding._invokeFrameCallback
package:flutter/…/scheduler/binding.dart:1175
#11     SchedulerBinding.handleDrawFrame
package:flutter/…/scheduler/binding.dart:1104
#12     SchedulerBinding._handleDrawFrame
package:flutter/…/scheduler/binding.dart:1015
#13     _invoke (dart:ui/hooks.dart:148:13)
#14     PlatformDispatcher._drawFrame (dart:ui/platform_dispatcher.dart:318:5)
#15     _drawFrame (dart:ui/hooks.dart:115:31)
════════════════════════════════════════════════════════════════════════════════

════════ Exception caught by widgets library ═══════════════════════════════════
Multiple widgets used the same GlobalKey.
════════════════════════════════════════════════════════════════════════════════

...

Analyzing app...
No issues found! (ran in 21.8s)

[✓] Flutter (Channel stable, 3.3.0, on macOS 12.5 21G72 darwin-x64, locale en-US)
    • Flutter version 3.3.0 on channel stable at /Users/tony/flutter
    • Upstream repository https://github.com/flutter/flutter.git
    • Framework revision ffccd96b62 (3 weeks ago), 2022-08-29 17:28:57 -0700
    • Engine revision 5e9e0e0aa8
    • Dart version 2.18.0
    • DevTools version 2.15.0

[✓] Android toolchain - develop for Android devices (Android SDK version 30.0.1)
    • Android SDK at /Users/tony/Library/Android/sdk
    • Platform android-31, build-tools 30.0.1
    • Java binary at: /Applications/Android Studio.app/Contents/jre/jdk/Contents/Home/bin/java
    • Java version OpenJDK Runtime Environment (build 1.8.0_242-release-1644-b3-6915495)
    • All Android licenses accepted.

[✓] Xcode - develop for iOS and macOS (Xcode 13.4.1)
    • Xcode at /Applications/Xcode.app/Contents/Developer
    • Build 13F100
    • CocoaPods version 1.11.2

[✓] Chrome - develop for the web
    • Chrome at /Applications/Google Chrome.app/Contents/MacOS/Google Chrome

[✓] Android Studio (version 4.1)
    • Android Studio at /Applications/Android Studio.app/Contents
    • Flutter plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/9212-flutter
    • Dart plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/6351-dart
    • Java version OpenJDK Runtime Environment (build 1.8.0_242-release-1644-b3-6915495)

[✓] VS Code (version 1.71.1)
    • VS Code at /Applications/Visual Studio Code.app/Contents
    • Flutter extension version 3.46.0

[✓] Connected device (3 available)
    • Tony Xr (mobile) • 00008020-000C11A23C02003A • ios            • iOS 15.4.1 19E258
    • macOS (desktop)  • macos                     • darwin-x64     • macOS 12.5 21G72 darwin-x64
    • Chrome (web)     • chrome                    • web-javascript • Google Chrome 97.0.4692.71

[✓] HTTP Host Availability
    • All required HTTP hosts are available

• No issues found!

We are hitting this as well in a GoRouter setup when re-rendering the router using riverpod (because dependencies change); it's rather frustrating and borderline impossible to debug :/

I've been hitting this same issue for the past days. I use a ShellRoute with a pageBuilder that, depending on the route, uses a CustomTransitionPage with no transition or one with a FadeTransition. When I comment out the use of the FadeTransition, everything works fine. This makes me believe that the issue maybe is caused due to the FadeTransition showing two routes simultaneously, cross-fading between them. Does that sound plausible?

We are hitting this as well in a GoRouter setup when re-rendering the router using riverpod (because dependencies change); it's rather frustrating and borderline impossible to debug :/

Hey, I think I have the same issue too. I too use a similar setup that's based on Riverpod provider and GRouter. Any solutions that you found out?. Thanks in advance.

I have the same error

I have the same error

Can you try create a static variable routeConfig like this.
`
class AppRouter {
static GoRouter _routerConfig() {
...
}
}
static final _routerConfig = AppRouter().routerConfig;
@override
Widget build(BuildContext context) {
return MaterialApp.router(
// router
routerConfig: _routerConfig,
);
}
`

We are hitting this as well in a GoRouter setup when re-rendering the router using riverpod (because dependencies change); it's rather frustrating and borderline impossible to debug :/

Hey I was able to escape the error, by including the global key declarations inside the routerProvider definition itself. So that each time a change occurs, the router body would rerun and different keys would be supplied. I am not sure whether am I right or wrong, but I was able to escape this issue.

@yknevenky I ran into the same issue while using Riverpod with GoRouter. I believe moving the GlobalKey declarations insider the provider is the right fix!

This is likely related to, or a duplicate of, issue #148768. And possibly not a go_router issue, but instead an issue in Flutter (Navigator).


Add ECDSA support to the aztec3-hacky branch

Cherry-pick the commits needed for ECDSA support to the aztec3-hacky branch, so we can use it from Noir contracts in Aztec.

Tagging @sirasistant

Clippy errors originate from the branch itself, so this is fine.

For reference + context; ECDSA was broken in the barretenberg backend and recently got fixed.
Since it is not a Noir only error, we will also need to point to the version of barretenberg that introduces this fix. This is likely why the tests won't pass.
We also cannot point to the latest barretenberg because I think this includes breaking changes between the version that the hacky branch points to and the version with the ECDSA fix.
It is definitely possible to patch an old version of barretenberg too and point to a branch in barretenberg, let me know if this is something you wish to pursue

FWIW ECDSA is working fine here using this branch.

FWIW ECDSA is working fine here using this branch!

Oh thanks for the correction :)
I'm not quite sure why this works then since I also had to make a change in barretenberg too to make the verifier not panic + a few other miscellaneous fixes by Zac. If it's working on your end for the hacky branch though, then please feel free to ignore me :)


Fix: Ledger THOR support (after hard fork)

Want
Restore logic similar to what we have before introducing latest cosmos-client + xchain-cosmos|thorchain (compare changes in PR 2150 https://github.com/thorchain/asgardex-electron/pull/2150/files + check thread in https://discord.com/channels/838986635756044328/850210159636512801/955710569158164510)
TODOs

Provide json specified in https://github.com/LedgerHQ/app-thorchain/blob/master/docs/TXSPEC.md to pass it into app.sign
Send tx to txs endpoint
Remove scripts/fix.sr25519.js

Resolved by  #2159


[issue-5939] upgrade commons-compress to 1.21

Description
This PR...

Fixes: #5939

Types of changes

[ ] Breaking change (fix or feature that would cause existing functionality to change)
[ ] New feature (non-breaking change which adds functionality)
[X] Bug fix (non-breaking change which fixes an issue)
[ ] Enhancement (improves an existing feature and functionality)
[ ] Cleanup (Code refactoring and cleanup, that may add test cases)

Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale

[ ] Major
[X] Minor

Bug Severity

[ ] BLOCKER
[X] Critical
[ ] Major
[ ] Minor
[ ] Trivial

Screenshots (if appropriate):
How Has This Been Tested?

@pjfanning can you change the base branch to 4.16 and rebase your PR branch to 4.16?

@rohityadavcloud I'm going to close this - master and 4.16 are so different that a rebase is taking forever - I'll create a new PR from scratch

@rohityadavcloud I created https://github.com/apache/cloudstack/pull/5942


scc and fm low volume

Could the scc/fm volume be slightly increased from tang nano? With the jumper installed the psg sound from the msx has a much higher volume.
Thanks in advance.

can be solved if psg plays via jack 3.5 mixed with scc.


Ability to "declare" stubs as global functions

There's probably a good opportunity to replace usage of isolator with a strategy similar to php-mock. That is, declaring global functions in a namespace to exploit PHP's function resolution rules, and to swap out real global functions with stubbed and spied implementations.
I'm thinking something like this:
namespace My\Namespace;

function rollDice()
{
    return mt_rand(1, 6);
}

$stub = stub('mt_rand');
$stub->with(1, 6)->returns(6);
$stub->declareAs('My\Namespace\mt_rand');

$result = rollDice();

assert(6 === $result);
$stub->calledWith(1, 6);

In the above example, My\Namespace\mt_rand is declared, but its implementation can later be replaced by any stub that declares itself with the same name. That may take some interesting trickery.
Thoughts @jmalloc, @darianbr, @koden-km, @parkertr?

Could probably add a stubGlobal() convenience method that also makes the stub return null by default, a bit like a full mock (stubs don't default to this behavior, only mocks):
namespace My\Namespace;

use function Eloquent\Phony\stubGlobal;

$stub = stubGlobal('file_get_contents', __NAMESPACE__);

assert(null === file_get_contents('/path/to/secrets'));
$stub->calledWith('/path/to/secrets');

I'm sure of any technical nuances here but I very much like this feature.

Erin, notes as per discussion on "stub scope criteria" below :) It would not be necessary to implement any of this as the initial offering, if pursued at all.
Rationale
While, IMO, this system described in this issue is superior to an isolator, especially in terms of performance, there is some degree of flexibility lost.  We are currently able to create multiple isolators with separate behaviour in each instance. This sometimes becomes necessary when tests require multiple instances of the SUT.
By allowing stubs to match on the scope of the caller we can give them differing behaviour depending on where they are invoked.  This is especially useful with the "global stubs" feature where we have limited control over the (re)definition of the global PHP functions.
Stub criteria
All stubs would gain a method for adding a "calling scope" criteria.  If I have the terminology correct, this criteria limits the following stub answers to only those calls made inside a stack frame with a matching scope.  The scope could be a global function, a static or instance method, a specific class, or a specific instance of a class.
Such a criteria would be defined something like the following:
$stub->withCaller($functionOrClassOrInstance, $methodName = null)->does(...)
I suspect it wont be necessary to resolve the meaning of the first parameter ahead of time, rather this could be deferred until the calling stack frame (obtained via debug_backtrace) is inspected.
Global stubbing convenience parameters
The stubGlobal function described in the comment above would gain the criteria parameters:
stubGlobal($fn, $namespace, $functionOrClassOrInstance = null, $methodName = null)
If these parameters are omitted the function would behave as described above. If at least $functionOrClassOrInstance is provided, the "global stub" would instead dispatch to a "child" stub only when this criteria is met.  This "child" stub would be returned by stubGlobal, allowing the user to independently configure the behaviour without having to be aware of the scope criteria.
By way of example:
function stubGlobal($fn, $namespace, $functionOrClassOrInstance = null, $methodName = null) {
    $stub = /* create stub using "existing" behaviour */

    // there is no scope criteria, return the global stub
    if ($functionOrClassOrInstance === null) {
        return $stub;
    }

    // wire the global stub to dispatch to a child stub when the criteria is matched
    $child = Phony::stub($fn);
    $stub->withCaller($functionOrClassOrInstance, $methodName)->does($child);

    return $child;
}

In summary
... yeah it's madness.

I'm in favour of the withCaller() idea in general, but I think it's probably better to accept standard PHP callables in a single parameter, rather than $functionOrClassOrInstance and $methodName:
$stub->withCaller($closure);
$stub->withCaller('functionName');
$stub->withCaller('ClassName::methodName');
$stub->withCaller(['ClassName', 'methodName']);
$stub->withCaller([$instance, 'methodName']);

$stub = stubGlobal('file_get_contents', __NAMESPACE__, [$instance, 'methodName']);

Particularly since Phony already deals with these kinds of callable definitions in other places. The only question then is the semantics of how to indicate that any method from a particular instance or class should be matched. Perhaps an asterisk:
$stub->withCaller([$instance, '*']);
$stub = stubGlobal('file_get_contents', __NAMESPACE__, [$instance, '*']);

?

I'm not opposed to that, but one thing to consider is that they aren't quite callable, so you wont be able to type hint as such. Specifically, when you want to refer to a non-static method, but match any instance, could might refer to this as 'ClassName::nonStaticMethod' or ['ClassName', 'nonStaticMethod'], but that would be confused for a static method by PHP.  Hopefully that makes sense.

Can't type hint anyway - 5.3 support.

I LOL'D.

Oops, I didn't see your part about [$instance, '*'], I figured you'd just pass the instance itself, but that discounts closures and other invocables, so yep I'm on board with that.

I just realised that by-reference arguments make this more complicated. I'm going to have to define the "hook" function with the same parameters as the underlying stub in order to get them working. That means you can't switch out the implementation unless the function signatures are compatible.
Unfortunately I feel that it's a must-have. Many functions that are prime candidates for this feature take arguments by reference (i.e. exec()'s $output parameter).