Repository navigation
Introduce Qt6-based implementation for TCanvasImp - #22736
Conversation
5f7c92a to
bf253b8
Compare
TCanvasImp
silverweed
left a comment
There was a problem hiding this comment.
I started reviewing this PR and made it about 10 commits in, but it seems to me that this is still in a state not ready for review: there are several commits where something is introduced and later changed in other commits, as well as many leftovers of commented-out code and debug prints.
Please try reorganizing the commits in a more review-friendly manner to help understanding what is going on (and also to not leave many commits with an incomplete state in the history when this is merged)
|
I know that commits are not looking perfect - but they show the way how I develop the code. I use a lot code from our go4 project where QtRoot canvas with TVirtaulX-based graphics is used. Whats why in original commits code appears which than removed or significantly changed. Such big development will never be linear and clear. But this is the way how software is developed. |
This is such a mischaracterization of what I asked that I don't know what to say about it. Just, that PRs and commits in a team project should not look as they would if you were developing the software by yourself, otherwise whoever reviews this code will have a much harder time understanding it properly, which is the point of reviewing it in the first place. |
|
My point is - rewriting/reorganizing commits now makes no sense. There are fundamental problems which should be resolved and discussed:
All kind of debug output, commented out code, unused variables, compiler warnings can be resolved once such crucial points are proved to be working. |
Test Results 23 files 23 suites 3d 13h 48m 32s ⏱️ Results for commit 7037a68. ♻️ This comment has been updated with latest results. |
Support special fill patterns 3000..3025 reusing X11 pixmaps Implement box and fillarea painting
Once generic API exists to paint markers - use it here instead. There are 5 different places in ROOT which repeats again and again all markers implementation
It is not equivalent with TTF font and must be about 0.75
Use QString::toLatin1() method, otherwise special characters like used in symbols.ttf, are handled as utf-8 as displayed wrongly
Like press, move, release mouse button. On the right mouse button actiate context menu. For methods with arguments provide special dialog. Code partially copied from Go4 project and originally was developed in 2002 by Denis Bertini and improved later by Sergey Linev
It fully dismiss standard GUI classes and only allows to create Qt6canvas. Aim to provide specialized implementation for application and context menu Declare it in ROOT::Experimental namespace
It is implementation for context menu used inside qt6_canvas Idea to let handle right mouse button click in the canvas handler which will invoke implementation class
This is default place where all normal or custom items for the class are collected. By large copy code from TRootContextMenu.
Reuse several methods from TContextMenu to create labels for arguments and afterwards execute selected method with provided list of arguments.
These are: * open ROOT file, * Save as dialog * Menu items with supported formats Use QFileDialog, list of supported extensions can be moved later to base class
Implement toggling of empty QToolBar Implement toggling of status branch Implement tooltip event handling and toggling of such mode
For line, fill, text, marker attributes provide dialogs to avoid ROOT crash while default editors does not work in Qt For all attributes reuse colors editors with opacity
Required for zooming and corrects interactivity handling in ROOT
Add \class mark for all used classes Provide some remarks around QPainter and paintEvent handler Remove several debugging printf Make headers #ifdef with same syntax
Idea to move dedicated application initialization there
When TQt6Application is created immediately create also QApplication. In the future one can make it optional
Gives information to application that virtualx is not required.
Use new TAttMarker::GetMarkerShape() method
Prevent access to nullptr Prevent out-of-bound access to markers Remove timeout from mouse handling AI-assist with qwen llmbot
jblomer
left a comment
There was a problem hiding this comment.
In principle looks good to me! I went commit by commit, some comments are invalidated by later commits, feel free to ignore them.
Are the Q... classes visible as symbols? If so, I think they should go in ROOT::Internal (can be another PR).
|
I will merge PR and will try to create next one with my newer changes - TTF implementation and basic graphics editors. |
Many comments are valuable, but can be considered when final implementation will be done. Currently code changes rapidly, methods appears and then removed again. At its final stage TQt6PadPainter will be much more compact and lines which are discussed now simply disappear. Same with TQt6Canvas - lot of gVirtualX-related methods will be gone. Therefore I see no needs to already now make each commit and each comment perfect.
After setting
Gui.Factory: qt6TQt6Canvascanvas implementation will be created in ROOT.Painting with
TQt6PadPainterredirected toQPainterclass where all ROOT primitives -polyline, fillarea, polymarkers, line, box, text - are implemented.
Mouse click/move/release/wheel/enter/leave events are processed.
Menu implemented with
QMenuBar, toolbar withQToolBar, status line withQStatusBar.Also context menu is implemented providing
TQt6ContextMenuImpclass.It includes special dialog for input of method parameters.
Also toggle menu item with checked states should work.
Simple dialogs for line, fill, text and marker attributes are implemented.
For now they invoked when correspondent context menu command is activated,
later one can provide better integration via gui factory.
Provided implementation fully avoids
gVirtualXusage - pure Qt graphics is used.TQt6Canvasworks now with basic primitives likeTLatex,TLine,TBoxand with
TH1/TH2classes in 2D and 3D plots.Some ROOT classes may crash while they internally use
gVirtualXfor event handling.These classes will be fixed/improved later.
Classes declared in
ROOT::Experimentalnamespace. I re-wrote commits history (took 1.5 hours) to have classes defined from the beginning as experimental. These are:ROOT::Experimental::TQt6CanvasROOT::Experimental::TQt6PadPainterROOT::Experimental::TQt6GuiFactoryROOT::Experimental::TQt6ApplicationThere are know problems/issues/open questions:
TApplicationinvokes qt events per timer, but one can have opposite situation when ROOT classes used fromQApplication. At least our API should be capable to handle both situations.gVirtualX/gClientinitialization completely, now only gClient is skippedTGuiFactorycan create them, but do we want to implement them for alternative graphics backends; do we want to reuse TRootBrowser or RBrowser code?root --backend=qt6; there are more backends expectedgVirtualXimplementation, need to be investigatedThis is first (but not the only) new graphical backend which can fully replace
gVirtualX-based implementation.After finalizing of API some other - more simpler (like raylib) or more advanced (like Vulkan) - can be exercised.