-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[main] replace rootbrowse.py with C++ version #19722
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
28f1346
to
797cf93
Compare
main/src/rootbrowse.cxx
Outdated
options: | ||
-h, --help show this help message and exit | ||
-w, --web WEB Configure webdisplay. For all possible values, see: | ||
https://root.cern/doc/v636/classTROOT.html#a1749472696545b76a6b8e79769e7e773 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if there is a way to point to the latest doc consistently...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be great! Or this string needs to be updated as part of the release procedure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://root.cern/doc/master/classTROOT.html#a1749472696545b76a6b8e79769e7e773
works (maybe, I am not sure how stable the hash is).
@dpiparo It would also be nice if
https://root.cern/doc/latest-stable/classTROOT.html#a1749472696545b76a6b8e79769e7e773
worked ...
PS. Apologies to @ferdymercury who already proposed this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With Danilo we decided we will do this, however we need a good way to make it obvious which version a user is looking at, and not just "latest-stable". This will probably need a little work on the website js - I will do it as soon as I manage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not doing it on the c++ side or even CMake side? since R__VERSION_MAJOR is known, we can decide whether the help prints:
v636 or v634 or master, etc.
For non-even dev versions, either "master" or round-up to even number.
This could be a generic CMake or C++ function that would be useful for everybody, not just rootbrowse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't hate the idea of having a latest-stable
link, regardless of what we decide to do with this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test Results 21 files 21 suites 4d 0h 57m 15s ⏱️ For more details on these failures, see this check. Results for commit b717f19. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
main/src/rootbrowse.cxx
Outdated
options: | ||
-h, --help show this help message and exit | ||
-w, --web WEB Configure webdisplay. For all possible values, see: | ||
https://root.cern/doc/v636/classTROOT.html#a1749472696545b76a6b8e79769e7e773 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be great! Or this string needs to be updated as part of the release procedure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reporting from my MacOS 15 machine:
- calling
rootbrowse
by default starts the webgui, which just hangs and does not produce anything on screen:
08:56:03 (rootdev) vpadulan@vpadulan-macbook rootproject → rootbrowse
ROOT comes with a web-based browser, which is now being started.
Revert to TBrowser by setting "Browser.Name: TRootBrowser" in rootrc file or
by starting "root --web=off"
Find more info on https://root.cern/for_developers/root7/#rbrowser
Info in <THttpEngine::Create>: Starting HTTP server on port 127.0.0.1:8846
Press ctrl+c to exit.
- setting "Browser.Name: TRootBrowser" in rootrc does not work,
rootbrowse
does not pick up the option and still tries to start the web graphics
@linev the fact that the web graphics hang on MacOS is troublesome
@vepadulano I see no problem on my Mac with master branch.
or
It is troublesome that simple commands is not know to everybody. |
What command do you mean specifically? I'm just testing |
main/src/rootbrowse.cxx
Outdated
// NOTE: we need to instantiate TApplication ourselves, otherwise TBrowser | ||
// will create a batch application that cannot show graphics. | ||
TApplication app("rootbrowse", nullptr, nullptr); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General comment on why I never use rootbrowse:
You sometimes come to a point where you would like to type something in the terminal and you cannot.
My question: wouldn't it be easier to just call, as an alias:
root -l %arg1 --web=%arg2 -e 'auto br = new TBrowser;'
rather than creating a TApplication ?
Or, alternatively, create a TRint instead of a TApplication, as in rmain.cxx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this would indeed be a better option. Begs the question why it wasn't done like that so far though - if there are reasons for it I am not aware of them...
Old There is PR which provides reasonable solution for python on all platforms - including Windows and Mac. It is pending since January. |
@linev Vincenzo was referring to this version of rootbrowse, so Python is not involved. We later found that the web graphics was not actually "hanging", but rather opening a window in another desktop without moving automatically to it - this is obviously better than hanging, but still not a great experience for a user. Is it expected to do that or should the web window also get focused automatically (like the classic graphics do)? |
c4a9972
to
99d004f
Compare
I do not have control over desktop applications when starting external browser via the shell functionality. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on my MacOS ARM laptop, works with both traditional graphics and web graphics, thanks!
Similarly to #19305 with rootls, this PR replaces
rootbrowse.py
with a native version.Since rootbrowse is just a thin wrapper over TBrowser, this change is much smaller than for rootls and should be quite straightforward.
NOTE: the new rootbrowse is currently untested on Windows and Mac, so before merging this I would much appreciate if someone could help me with that.
Checklist: