Creating child window for rendering by digiperfect-tech · Pull Request #285 · highperformancecoder/minsky

digiperfect-tech · 2021-02-17T10:37:29Z

We have created a struct WindowInformation that stores the childWindowId along with other details like display and window attributes. This information is reused across multiple calls to renderFrame.

The flow for code will be -- when minsky starts:
A call to/minsky/canvas/initializeNativeWindow will be made, with parentWindowId (and offsets) as the parameters (creating child window in electron did not work as expected, so we need to work with offsets).
Subsequent repaints can be requested with /minsky/canvas/renderFrame

As of now, we create a cairo surface with each call to renderFrame, though I think the surface can also be reused. I tried having pointer to cairo::SurfacePtr (not sure we should have pointer to pointer) but it didn't work as expected (maybe syntax related usage issue), so for now I reverted to recreating the surface in the call to renderFrame

Please especially review the lifecycle (constructors, desctructors and copy constructors) that I have defined in renderNativeWindow.cc . I think the WindowInformation object that is destroyed in the destructor for RenderNativeWindow can also be reused (perhaps it can be made a static object?).

Also - am not sure how to distinguish between destructor for RenderNativeWindow that will be called with each call to load model (or undo/redo as you mentioned), and the final call when minsky is closed.

This change is

…ints

highperformancecoder · 2021-02-18T10:38:04Z

model/renderNativeWindow.cc

-  
-  inline cairo::SurfacePtr nativeWindowSurface(unsigned long window)
+
+  inline cairo::SurfacePtr createNativeWindowSurface(WindowInformation *wi)


There's no good reason to pass this as a pointer. Better to use a reference.

highperformancecoder · 2021-02-18T10:46:24Z

model/renderNativeWindow.h

  {
  public:
-    void renderToNativeWindow(unsigned long window);
+    /* Perhaps needs to be public for some auto-binding code */


Definitely not. If you're managing raw pointers in this way (new in constructor/delete in destructor), then the actual pointer must be private, and the copy operations must be deleted. But most classes in Minsky need to be DCAS (default constructible, copy constructible, assignable and serialisable), so you won't get very far with this strategy.

If you do need pointers (I'm not sure you really need them here, mind you), then using smart pointers is the smart thing to do, as all these issues have been thought out for you. For example, std::shared_ptr is DCAS, and std::unique_ptr is nearly so.

highperformancecoder · 2021-02-18T10:48:47Z

model/renderNativeWindow.cc

+    cout << "Delete called for window information" << endl;
+  }
+
+  void WindowInformation::initialize(unsigned long parentWin, int left, int top, int cWidth, int cHeight)


This should really be implemented in the constructor (RAII and all that).

highperformancecoder · 2021-02-18T10:49:49Z

model/renderNativeWindow.cc

+  void WindowInformation::copy(WindowInformation *winInfo)
+  {
+    this->parentWindowId = winInfo->parentWindowId;
+    this->childWindowId = winInfo->childWindowId;


childWindowId is managing resources in just the same way as a pointer does, so this line leaks resources. The proper way to solve this is to apply RAII.

highperformancecoder · 2021-02-18T10:52:38Z

model/renderNativeWindow.h

+    int offsetTop;
+    Display*	display;
+    XWindowAttributes wAttr;
+    ecolab::cairo::SurfacePtr *childSurface; // TODO:: Review right way to store surface ptr


A SurfacePtr is a smart pointer - basically a std::shared_ptr IIRC. Taking a pointer to it will almost invariably lead to resource leaks and object lifetime problems.

highperformancecoder · 2021-02-18T10:54:16Z

model/renderNativeWindow.h

+    int childHeight;
+    int offsetLeft;
+    int offsetTop;
+    Display*	display;


Because of the problems pointers create, it is worthwhile noting that this is a weak pointer, ie ownership of the display object remains with the X11 system.

highperformancecoder · 2021-02-18T10:59:06Z

model/renderNativeWindow.cc

+  {
+    auto tmp = createNativeWindowSurface(winInfo);
+
+    //TODO:: Review if this paint (below 3 lines) is really needed with each frame


My intuition is that painting the background canvas is not necessary for X11, but might be for Windows. I guess we should suck it and see.

highperformancecoder · 2021-02-18T11:15:56Z

We have created a struct WindowInformation that stores the childWindowId along with other details like display and window attributes. This information is reused across multiple calls to renderFrame.

The flow for code will be -- when minsky starts:
A call to/minsky/canvas/initializeNativeWindow will be made, with parentWindowId (and offsets) as the parameters (creating child window in electron did not work as expected, so we need to work with offsets).

Why not check whether windowInfo is initialised in renderNativeWindow, and initialise if not at that time? Lazy initialisation.

Subsequent repaints can be requested with /minsky/canvas/renderFrame

As of now, we create a cairo surface with each call to renderFrame, though I think the surface can also be reused. I tried having pointer to cairo::SurfacePtr (not sure we should have pointer to pointer) but it didn't work as expected (maybe syntax related usage issue), so for now I reverted to recreating the surface in the call to renderFrame

Yes - the SurfacePtr needs to have a shorter lifetime (only just shorter) than the child Window. So it should probably be declared after the WindowInfo to ensure it is destroyed before the child window.

Please especially review the lifecycle (constructors, desctructors and copy constructors) that I have defined in renderNativeWindow.cc . I think the WindowInformation object that is destroyed in the destructor for RenderNativeWindow can also be reused (perhaps it can be made a static object?).

Yes - lots of problems, as I identified in the comments. Please review RAII principles.

Also - am not sure how to distinguish between destructor for RenderNativeWindow that will be called with each call to load model (or undo/redo as you mentioned), and the final call when minsky is closed.

There is no need to distinguish these (it would be a mistake to do so).

highperformancecoder · 2021-02-22T23:17:23Z

model/renderNativeWindow.cc

+    if (this->winInfo)
+    {
+      delete this->winInfo;
+    }


This still won't do. RenderNativeWindow needs to be DCAS, for Classdesc reflection reasons.

highperformancecoder · 2021-02-22T23:20:26Z

model/renderNativeWindow.cc

-    auto tmp=nativeWindowSurface(window);
+    //TODO:: I expected this to be the "new" instance with uninitialized winInfo and a to have the previous winInfo.
+    // However, it seems to be the other way round in practice --- Janak
+    a.winInfo->initialize(*(this->winInfo));


In C++, just because a pointer is const, doesn't mean the target of the pointer is const. It's a little counterintuitive, but you have to add extra qualifiers to the type to ensure the target is const,

Nevertheless, it is is bad form to go changing the RHS of an assignment statement - it would be totally surprising to most C++ programmers.

highperformancecoder · 2021-02-22T23:24:59Z

model/renderNativeWindow.h

+  private:
+    CLASSDESC_ACCESS(RenderNativeWindow); 
+    // std::shared_ptr<WindowInformation> winInfoPtr; // TODO:: cannot get things to work with this
+    WindowInformation* winInfo;


As mentioned, RenderNativeWindow should be DCAS, in particular have valid copy and assignment operators. Best to arrange all members of RenderNativeWindow be DCAS, and then take the compiler generated copy and assignment operators - this is more scalable.

I think we have two options for making the winInfo member DCAS - the shared_ptr approach I outlined, and the no-op copy operations: both have their own set of issues and implications. Maybe other methods will work, but I can't think of any.

As for the 'S' part, a simple Exclude wrapper is sufficient.

highperformancecoder · 2021-02-22T23:28:04Z

model/windowInformation.cc

+    // Todo:: currently width, height parameters are not getting passed properly
+    // Eventually, we need those in order to ensure we don't need to worry about
+    // paddings, scrollbars etc


Why wouldn't you arrange for parentWindow to be a dedicated childWindow within electron, that way there will be no scrollbars or padding involved?

model/windowInformation.cc

highperformancecoder · 2021-02-22T23:35:41Z

model/windowInformation.h

+
+    ecolab::cairo::SurfacePtr getSurface();
+
+    void copyFrom(const WindowInformation &other);


I don't see the implementation of this method here, but I'm concerned you're trying to introduce a copy operator in by the back door, and no correctly handling resource lifetime (like the initialize() example above).

highperformancecoder · 2021-02-24T05:39:07Z

model/renderNativeWindow.h

-    void renderToNativeWindow(unsigned long window);
+  private:
+    CLASSDESC_ACCESS(RenderNativeWindow); 
+    std::shared_ptr<WindowInformation> winInfoPtr;


I think you will probably have to classdesc::Exclude this member.

highperformancecoder · 2021-02-24T05:45:07Z

Visually, the code now passes muster. You also need to for the code to pass continuous integration. Looking at the errors Travis flags, I'd say this can be dealt with by wrapping winInfoPtr in a classdesc::Exclude construct.

I'll try checking out this PR branch and seeing if that does the trick.

digiperfect-tech added 6 commits February 16, 2021 16:19

Initial rendering code commit

3d7408b

Struct for WindowInformation and reusing the child window across repa…

1733fe1

…ints

offset reversal fixed

c119dd7

Added processing of offset arguments, though they don't work

d14090c

winInfo made public

01eeda1

function placeholder

9131626

highperformancecoder reviewed Feb 18, 2021

View reviewed changes

WindowInformation in separate file

446d6e3

highperformancecoder reviewed Feb 22, 2021

View reviewed changes

model/windowInformation.cc Show resolved Hide resolved

highperformancecoder reviewed Feb 22, 2021

View reviewed changes

digiperfect-tech added 2 commits February 23, 2021 15:36

Pass parentWindowId and offsets etc with each call to renderFrame

2be7a38

Moved surface and all window bookkeeping logic to WindowInformation

ac55587

highperformancecoder reviewed Feb 24, 2021

View reviewed changes

highperformancecoder merged commit ac55587 into highperformancecoder:RESTService-branch Feb 24, 2021

Copilot AI mentioned this pull request Mar 12, 2026

Export plot title to images. For #1259 #625

Open


		inline cairo::SurfacePtr nativeWindowSurface(unsigned long window)

		inline cairo::SurfacePtr createNativeWindowSurface(WindowInformation *wi)


		ecolab::cairo::SurfacePtr getSurface();

		void copyFrom(const WindowInformation &other);

Uh oh!

Conversation

digiperfect-tech commented Feb 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

highperformancecoder commented Feb 18, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

highperformancecoder commented Feb 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

digiperfect-tech commented Feb 17, 2021 •

edited

Loading