dropdown lists not working in widgets

[kaamui]

Like for hover events, there is no issue on the navigator, but on widgets, some events seem to not be received or even sent when user clicks on a dropdown list.

I saw in the event filter that a ChildAdded and activateWindow events are received, but can't find what to do with it. I could reproduce it on Windows and Linux, on every version of Qt I have. Quite strange I didn't notice such a thing earlier.

Can you confirm it too ? a "No" would be a great answer 😄

I also reproduced it with your minimal code, using this page for tests => https://www.w3schools.com/tags/tryit.asp?filename=tryhtml_select

Do you have any clue on what could be happening ? Any workaround in mind ?

[letsfindaway]

I remember an early problem that the popup was displayed in a totally wrong location. See #46. And indeed with Qt 5.15 and Qt 6.2 I have this problem: Note: I made the main frame a little big larger to get this picture.

With Qt 6.4 the menu appears below the drop down. But when I move the window and then try it again, then the menu appears still at the "old" position. This is very much like #46.

With OpenBoard I cannot see the menu at all for Qt 5.15 and 6.2, so I must confirm that issue. Have not tried with 6.4.

My workaround in #46 seems also to circumvent some missing event forwarding by resizing. Probably this does no longer work, so we need another workaround. I might have some time over the weekend to look into that.

[kaamui]

Quite a good news that the child item is showing, if we can manage to have it showing "not too far" from the correct position ^^

Do you know what's the commit related to #46 ?

[letsfindaway]

The commit related to #46 is 2faf67f3423aa991676dbdd7d8f4b95a60470103. After applying some patches to the project file I can also compile it. And: the menu appears, however at a slight offset:

I will now do a git bisect to find the point when the menus have vanished.

[letsfindaway]

git bisect told me that commit 930b737c3c7585213916df6b49fb0d95c0f5fb1d, which is the merge commit when merging refactor-webengine is the first commit where the drop down menus are not working. Before they are working on my branch (however with some offset as shown above) as well as on the original webkit based branch.

I will now look and see what might have happened during this merge.

Edit: I cannot see anything. Will now double check with those two commits.

Edit 2: Indeed: when checking out at commit 6f036036d8328a1f37f698bb5acdac0af030cd06 it works. At the immediately following merge commit linked above the drop down menu does not appear.

If you want to see what has changed between these two commits, see the comparison here https://github.com/letsfindaway/OpenBoard/compare/6f036036d8328a1f37f698bb5acdac0af030cd06...930b737c3c7585213916df6b49fb0d95c0f5fb1d. It is about thumbnails, threaded persistence, the new singleapplication, the tree view in document mode, but nothing in the web directory and only a marginal change in domain.

[letsfindaway]

Further git bisect on the commits added to refactor-webengine by the merge, it turned out that the menu does no longer show up after the switch from UBDocumentNavigator to UBBoardThumbnailsView in commit 7e2e08bf7ca21bde17e168fcaa576be497f2e47e.

What I did: I checked out the topmost commit from refactor-webengine, created a new branch here and tried to add all commits introduced by the merge to that branch using git cherry-pick. This did not work to the end, but I found that the error was introduced in the range of commits successfully picked. Then I did a git bisect on that range. This gave me finally the following picture: You see that the commit are now linearly added. The bottom commit is a minor adjustment for builds on my computer (quazip, you know).

What happens here is that an additional view is attached to the scene, the UBThumbnailView. Could this be somehow related? Just for testing I made this little modification:

UBThumbnailView::UBThumbnailView(UBGraphicsScene *scene, QWidget* parent)
    : QGraphicsView(nullptr, parent)    // nullptr instead of scene
    , mHBoxLayout(new QHBoxLayout(this))

Instead of passing the scene to QGraphicsView i just passed nullptr. And yes, then the menu appears again.

Now going with the dog, after that I will check whether this detaching from the scene has the same effect on the current dev. If yes, then we at least know what lead to that bug. Still no clue why it happens and how to avoid.

[letsfindaway]

Now back from my walk and testing with the current dev.

• When I detach the scene from the UBThumbnailsView as shown above, then the menu appears, even at the correct location.
• When I add a second monitor, and by that another view on the display screen, then the menu again does not work.

So the bug is indeed related to the number of views attached to the scene, and rendering the menu does not work when more than one view is attached. Bad. We don't have this problem in web view. Here the menu correctly appears on the control and the display screen. But this is a different situation.

In which way could we look for a solution?

We could only attach a single view (the control view) to the scene and to use screen grabbing to mirror the content to the display view and the thumbnail view. This can probably be automated so that each time the control view is painted, the other views get updated. We would then duplicate the view on the widget level, invisible to the items of the scene.

This seams to be feasible if all views have the same zoom factor and panning position. For control and display view this is the case. For the thumbnail it would also not be a problem, I think. However the aspect rations may be different. With this solution we cannot e.g. on a 16:9 display view render a wider area of the scene as on a 4:3 control view. Instead we could only scale it to show a subset. We can also not cover different screen resolutions. This seems to be the major drawback of this approach.

We can also render a scene to a paint device using QGraphicsScene::render without attaching a view. Perhaps we can also use this to show the scene in the thumbnail and display view. However we are then responsible to find the proper point in time to do this. Perhaps we can use the PaintEvent delivered to the control view or the changed signal from the scene to trigger this in order to avoid a cyclic timer.

We should also put some effort in investigating why attaching a second view to the scene has such side effects and try to find a workaround.

Also a minimal demonstrator of the problem could be useful if we want to open a Qt bug report.

[letsfindaway]

Proposal for solution

Assuming that it is not so easy to actually fix this I will start with the following proposal for finding a solution:

• Create a minimal proof-of-failure, just as for the cursor issue.
• Create a new UBSceneWidget class acting as a read-only view of a scene using QGraphicsScene::render without attaching a view. See below for details.
• Use this class for the live thumbnails as well as for the display screen.

Of course at each step I make some assumptions. First that the minimal proof-of-failure actually shows the problem. second that the UBSceneWidget actually solves it.

UBSceneWidget

This class will work as follows:

• It inherits from QWidget.
• It can be used as base class of UBThumbnailView instead of QGraphicsView.
• It can be used as mDisplayView in UBBoardController.
• It connects to QGraphicsScene::changed() to detect changes on the scene.
• It uses QGraphicsScene::render() to render the updated areas on the widget.

Still nothing of this proposal has been proved. It will however also fix the "problem" that you could interact with the scene on the thumbnails. As UBSceneWidget will be strictly a one-way rendering solution, there is no event processing necessary at all. It will be much more lightweight than the QGraphicsView, as it does no event processing, but probably similar for rendering.

QGraphicsView also uses this update mechanism and connects QGraphicsScene::changed() to QGraphicsView::updateScene, at least as fallback to some more direct connection to the scene. We can use this slot as an example what we have to do here.

[letsfindaway]

First results

First, and probably most important result: The minimal example does not show this problem (checked on Qt 5.15). The workaround introduced for #46 is still necessary, but then the menu is appearing on multiple views of the same scene. This is good news, as it means that we can find a solution within OpenBoard instead of having to provide a workaround for a Qt problem.

Second, smaller result: you could inhibit interaction on thumbnails by simply setting setInteractive(false) on the UBThumbnailView. And a side note: The UBThumbnailView invokes setStyleSheet() twice. Note that the latter replaces the first. They are not additive.

[kaamui]

Thank you for the work you've done on this subject.

Instead we could only scale it to show a subset. We can also not cover different screen resolutions. This seems to be the major drawback of this approach.

The display view is way more important than the widgets or the thumbnails view, in terms of usage. If the team had to choose, because we can't find a solution, I think they would sacrifice real-time thumbnails views + widgets in order to preserve actual display view behavior.

[letsfindaway]

I'm now focusing on OpenBoard. For further analysis I completely disabled the thumbnail view and now only check with single/dual monitor setup. And probably one of my observations above war wrong: The menus work in single and dual monitor mode! Very good! The problem only occurs when the live thumbnails are added. So let me now step-by-step re-enable the thumbnails.

I already have seen: completely disabling the view by not creating a UBBoardThumbnailsView in the UBPageNavigationWidget solves the issue as well as not assigning a scene to the UBThumbnailsView as already tested above.

It does not help to completely comment out the body of UBThumbnailsView, so that it is more or less a plain QGraphicsView.

The live thumbnail view is a rather complex structure: The sidebar contains a QGraphicsView (the UBBoardThumbnailsView) which uses QProxyWidget to embed widgets containing another QGraphicsView (the UBThumbnailsView) containing again a QProxyWidget to embed the QWebEngineView`.

The items of the UBBoardThumbnailsView are of type UBDraggableThumbnailView which inherits from UBDraggableThumbnail which inherits from UBThumbnailProxyWidget which again inherits from QGraphicsProxyWidget.

Phew, not an easy structure.

I now did some experiments in this piece of code:

UBDraggableThumbnailView* UBBoardThumbnailsView::createThumbnail(UBDocumentProxy* document, int i)
{
    UBApplication::showMessage(tr("Loading page (%1/%2)").arg(i+1).arg(document->pageCount()));

    UBGraphicsScene* pageScene = UBPersistenceManager::persistenceManager()->loadDocumentScene(document, i);
//    auto tmp = new QGraphicsView(pageScene);
//    UBThumbnailView* pageView = new UBThumbnailView(pageScene);

    return new UBDraggableThumbnailView(nullptr, document, i);  // pass a nullptr
}

Passing the nullptr does not help. Now to the two commented lines. The lower is the original code. If I comment this line, then the menus work. So they fail by just creating this object. Then I tried the upper line: just create a plain QGraphicsScene. Then the menu appears, but not at the right position. So indeed, just creating these objects without using them anywhere affects the menus!

[kaamui]

I cannot reproduce the issue when testing this :

UBDraggableThumbnailView* UBBoardThumbnailsView::createThumbnail(UBDocumentProxy* document, int i)
{
    UBApplication::showMessage(tr("Loading page (%1/%2)").arg(i+1).arg(document->pageCount()));

    UBGraphicsScene* pageScene = UBPersistenceManager::persistenceManager()->loadDocumentScene(document, i);
    UBThumbnailView* pageView = new UBThumbnailView(new UBGraphicsScene(nullptr));

    return new UBDraggableThumbnailView(pageView, document, i);
}

It's strange that you don't have the same result when creating QGraphicsView and UBThumbnailView, while commenting the body (I suppose you meant commenting anything on the constructor) didn't help. Are you sure ?

It's quite odd that simply attaching the scene to a second view make everything works good except this. Maybe a QWebEngineView issue ?

The live thumbnail view is a rather complex structure: The sidebar contains a QGraphicsView (the UBBoardThumbnailsView) which uses QProxyWidget to embed widgets containing another QGraphicsView (the UBThumbnailsView) containing again a QProxyWidget to embed the QWebEngineView`.

The items of the UBBoardThumbnailsView are of type UBDraggableThumbnailView which inherits from UBDraggableThumbnail which inherits from UBThumbnailProxyWidget which again inherits from QGraphicsProxyWidget. The items of the UBBoardThumbnailsView are of type UBDraggableThumbnailView which inherits from UBDraggableThumbnail which inherits from UBThumbnailProxyWidget which again inherits from QGraphicsProxyWidget.

Phew, not an easy structure.

The constant need of QGraphicsProxy makes things a bit more complex than we would like, but except this, I find the structure quite easy (I don't like over complicated stuff at all, and it's one of the rare parts of the code I did myself from scratch so I may not be purely impartial :smile: but I find it really good and simple !) :

• the sidebar displays thumbnails of each page of the document so we can scroll through it => QGraphicsView (QAbstractScrollArea)
• each thumbnail is a draggable item (draggable => QGraphicsProxyWidget) composed of :
  • a view or a pixmap of the scene
  • a label of the scene index
  • a UI (to not duplicate data => UBThumbnailProxyWidget that simply gives access to the document)

[kaamui]

I can reproduce the issue with this code :

UBDraggableThumbnailView* UBBoardThumbnailsView::createThumbnail(UBDocumentProxy* document, int i)
{
    UBApplication::showMessage(tr("Loading page (%1/%2)").arg(i+1).arg(document->pageCount()));

    UBGraphicsScene* pageScene = UBPersistenceManager::persistenceManager()->loadDocumentScene(document, i);
    auto pageView = new QGraphicsView(pageScene);

    return new UBDraggableThumbnailView(pageView, document, i);
}

And I have the same behavior (can't see the dropdown list at all) as with the UBThumbnailView

[letsfindaway]

It's strange that you don't have the same result when creating QGraphicsView and UBThumbnailView, while commenting the body (I suppose you meant commenting anything on the constructor) didn't help. Are you sure ?

These were different situations. With the commended body I still attached it to the UBDraggableThumbnailView. So not comparable.

The constant need of QGraphicsProxy makes things a bit more complex than we would like, but except this, I find the structure quite easy (I don't like over complicated stuff at all, and it's one of the rare parts of the code I did myself from scratch so I may not be purely impartial smile but I find it really good and simple !) :

• the sidebar displays thumbnails of each page of the document so we can scroll through it => QGraphicsView (QAbstractScrollArea)

• each thumbnail is a draggable item (draggable => QGraphicsProxyWidget) composed of :

  • a view or a pixmap of the scene
  • a label of the scene index
  • a UI (to not duplicate data => UBThumbnailProxyWidget that simply gives access to the document)

I don't complain about this. I just say I needed some time to go through all of this. Some more comments and guidance in the code would probably have helped;)

My next try was to replace the nested QGraphicsProxyWidget by a class rendering the scene in a pixmap and displaying this. This looks as follows:

//------------
class XXSceneWidget : public QLabel
{
private:
    QGraphicsScene* scene;
    QTimer timer;

private slots:
    void updatePixmap()
    {
        QPixmap pixmap(200, 150);
        QPainter painter(&pixmap);
        scene->render(&painter);
        setPixmap(pixmap);
    }

public:
    XXSceneWidget(QGraphicsScene* scene)
        : scene(scene)
    {
        resize(200, 150);
        connect(&timer, &QTimer::timeout, this, &XXSceneWidget::updatePixmap);
        timer.start(200);
    }
};
//------------
UBDraggableThumbnailView* UBBoardThumbnailsView::createThumbnail(UBDocumentProxy* document, int i)
{
    UBApplication::showMessage(tr("Loading page (%1/%2)").arg(i+1).arg(document->pageCount()));

    UBGraphicsScene* pageScene = UBPersistenceManager::persistenceManager()->loadDocumentScene(document, i);
    XXSceneWidget* pageView = new XXSceneWidget(pageScene);

    return new UBDraggableThumbnailView(pageView, document, i);
}

Additionally I had to change the type of the UBDraggableThumbnailView::mThumbnailView to just a QWidget*, but this is easy, and the access function for that variable is never used, so it can be commented out.

The result of this hack is a working thumbnail view, however with some glitches, as it does not care about the size of the scene. But the menu works!!!

Should we go in this direction??

[kaamui]

I tested your hack, and indeed it solves the menu issue, but I think there is maybe too much counterparts to make it a direction we should take, at least for now.

The glitches and the size of the thumbnails not aligned with the scene, the FPS of the rendering (I'm worried about performances). .. do you think we could manage to get rid of the glitches and take the scene's size into account or is it impossible ? I think the FPS could be somehow an acceptable counterpart, but I'm pretty sure the "more visual" issues will be a no-go for the team.

I'm also worried about PDF rendering, that was asynchronously performed for each scene with the previous implementation. I think the need for the view to render it when the thumbnail was visible had the positive side effect to preload the PDFs for each one of them, would it still work the same with the painter (I think so, but your explanations on the low-level processes happening here would help me to understand a bit better the whole thing) ?

The other solution would be to revert the "real-time thumbnails" feature, but I'm not a big fan of it... but in the end having widgets working great is more important than having real-time thumbnails.

[letsfindaway]

I tested your hack, and indeed it solves the menu issue, but I think there is maybe too much counterparts to make it a direction we should take, at least for now.

The glitches and the size of the thumbnails not aligned with the scene, the FPS of the rendering (I'm worried about performances). .. do you think we could manage to get rid of the glitches and take the scene's size into account or is it impossible ?

This was just a very first hack! Of course the size stuff can be solved.

I think the FPS could be somehow an acceptable counterpart, but I'm pretty sure the "more visual" issues will be a no-go for the team.

I just replaced the timer with

        connect(scene, &QGraphicsScene::changed, this, &XXSceneWidget::updatePixmap);

and this works well for an update on demand. For a final solution even the QList<QRect> of changed areas provided by this signal can be taken into account for rendering to further improve performance.

I'm also worried about PDF rendering, that was asynchronously performed for each scene with the previous implementation. I think the need for the view to render it when the thumbnail was visible had the positive side effect to preload the PDFs for each one of them, would it still work the same with the painter (I think so, but your explanations on the low-level processes happening here would help me to understand a bit better the whole thing) ?

I still render each of the scenes at least once - so PDFs will be preloaded there.

About the low-level processes happening there: Let me try some explanations:

My proposed solution replaces an additional view on the scene by on-demand rendering into a pixmap. The thumbnails in the thumbnails view can then simply be labels containing these pixmaps, which are updated when needed.

When rendering a scene using QGraphicsScene::render the same processes take place as when rendering is requested by a view. So there is no difference for the items on the scene. Therefore I think this solution makes no difference for PDF rendering.

What the solution avoids is having a view with proxy items wrapping another view with proxy items, which might be beyond the things the Qt developers had in mind. Instead it breaks that nesting using the labels.

The other solution would be to revert the "real-time thumbnails" feature, but I'm not a big fan of it... but in the end having widgets working great is more important than having real-time thumbnails.

I don't think this is necessary.

I will try to provide a more elaborated version of that hack.

[letsfindaway]

See 110b5254478f129937f86c0095b3bf7fcc413503 for a proposed solution.

What I still don't like is the size of the delete and copy buttons, which are too large. I would propose to set (UBThumbnailWidget.h:643)

    const int ICONSIZE      = 32;   // instead of 96

I also see that sometimes the page label below the thumbnails is not fully displayed. Don't know whether this was before, but I have not touched anything about that. Can no longer reproduce this.

My branch is currently on top of my add-cmake-build-system branch, because I'm testing with that, but you can apply the commit as patch if you want to test. I'm typically rebasing such branches before I create a PR.

I think I'm not touching your structure at all, just very local changes. However a next logical step could be to replace the QPixmap in the QLabel wrapped in a QGraphicsProxyWidget by a QGraphicsPixmapItem. But this would then touch the current inheritance structure.

Currently missing is a better handling of empty pages, the centering of content or some other size handling, and the default background with scenes with dark background. But this is all some work, and nothing fundamental.

[letsfindaway]

About complexity: I have now drawn the current structure, ending at the labels representing the pages: Before my change the drawing was continuing to the right with a QGraphicsView rendering the QGraphicsScene of the page. And I constantly confuse the names. UBBoardThumbnailView, UBDraggableThumbnailView, UBThumbnailView sound similar, but still belong to different columns of that picture.

And I think the arrow from mThumbnailView to UBThumbnailView can now completely be removed, as it is now of type QWidget. The QGraphicsProxyWidget already manages this relationship.

[kaamui]

See https://github.com/letsfindaway/OpenBoard/commit/110b5254478f129937f86c0095b3bf7fcc413503 for a proposed solution.

I tried it. It's progressing. On my side I can confirm :

• At start, the size of the thumbnail of the empty scene is not corresponding to the size of the board.
• Importing an A4 document produces thumbnails with the standard "landscape" orientation, not corresponding to the actual ratio of the scene.
• icon size is way too big now and needs to be resized. 32 seems good
• default background when dark background is white (but probably fixed when the size handling is fixed ?)

Except the size handling, the result is promising. I hope we (almost you alone now, considering the speed at which you can produce results :)), manage to make the transition seamless for the test team, because they will probably reconsider live thumbnails if not. These little glitches are not the most blocking issues from a standard pov, but it's one kind of issue that particularly get reported by end-users when it happens, as it can really disturb the classroom while the teacher is trying to keep his/her students' attention.

[kaamui]

connect(scene, &QGraphicsScene::changed, this, &UBThumbnailView::updatePixmap);

wow, good idea. I'm surprised it doesn't seem to make a big difference in terms of performances. I thought producing the pixmap at each change would have a huge cost so I would not have tested it.

[letsfindaway]

I now pushed a second commit on that branch which makes the last column in my diagram unnecessary, because already the UBDraggableThumbnailView finally inherits from QGraphicsPixmapItem. This makes it also easier to solve the scaling issue. I now completely render the area of the pixmap, so that the background is always completely drawn.

I now also use the region parameter of the sceneChanged signal to render only the modified part of the scene. This improves performance, especially for pages with a lot of handwriting, i.e. many items, only a very small part of the page is changed.

As an experiment I also don't render thumbnails not visible in the pane. This speeds up loading of large documents. I have a 196 pages PDF document. Without this feature it takes 20 seconds to load the first slide in good resolution. With that, it only takes two or three seconds. As a side effect however this currently does not preload all the PDF pages, so some thumbnails don't show valid content when you scroll in the thumbnail pane as long as the associated page has never been visible on the board. My final idea is that the pages load as you scroll through the thumbnails.

But this optimization is also useful when you resize a thumbnail pane with many thumbnails, because the size change currently causes a re-rendering. Here I will probably improve the situation by only scaling the existing pixmap during resizing and doing the rendering only when the mouse button was released again.

Known bug: page numbers are not completely visible from time to time, e.g. when you use the hand tool on the board. Strange, but lets see.

And when we extend the used region by drawing outside of the nominal area of the page, then the delta updates are sometimes not correctly applied.

Note that this commit makes my previous commit unnecessary. These classes are not used any longer.

BTW: I will finally make a list of unused classes in this area. I know, you want to keep them, but I think it would be good to sort out which was used for what. And probably there is finally a candidate for removal ;)

We also need to test this on Mac with HiDPI display. probably some devicePixelRation handling is required when rendering.

[letsfindaway]

And here a more formal list of things to do, probably to be expanded:

• [x] Render pixmap fully when transformation has changed.
  A change of the transformation invalidates the complete pixmap, so it has to be redrawn completely. Note that in contrast to the previous implementation we now show the complete scene as returned by normalizedSceneRect() on the thumbnail, even if it extends the nominal size of the page. This causes the transformation to change when the used region of the scene grows or shrinks.
• [x] Decide how to handle portrait pages.
  Currently the thumbnails all have the same size, just the content is centered. before thumbnail with was adapted. How shall this finally be? See also the first point: We now render the complete scene, even parts outside of the "page size". What about his feature? Consider e.g. the document used to debug the PDF export scaling problem. It contained pages with the identical PDF background, but different annotations to the right or to the left. Here it would be very helpful to represent them on the thumbnail and not limiting it to the original page size.
  I have now seen that e.g. moving the eraser outside of the nominal area would be considered a scene change and would therefore also trigger re-scaling. At this level we cannot distinguish between tool and page content. So it would probably be better to use a fixed transformation.
• [x] Tell thumbnails whether they are exposed on the view.
  This can be used to render only those thumbnails which are at least partly visible. The UBBoardThumbnailsView can override scrollContentsBy and determine this from the new viewport. This makes it probably unnecessary to do something special when resizing, as now only the exposed thumbnails will be affected.
• [x] Render thumbnails completely which switch from unexposed to exposed.
  At least if they haven't been rendered before.
• [x] Fix the page number bug.
  How is the size of the text item calculated? Can't we just make it the same width than the thumbnails and then center the text? It seems that the page numbers are hidden behind some invisible whatsoever. Further to be analyzed. Does currently not occur. Fixed as some side effect??
• [x] Revert my first commit.
  it is no longer necessary.
• [ ] Move complex implementations from the UBThumbnailWidget.h header file to the corresponding .cpp file.
  This avoids compilation of all this code each time the header is included.
• [x] Break include hierarchy.
  UBThumbnailWidget.h is something very special, but due to the current include hierarchy it is even included by the completely unrelated UBSvgSubsetAdaptor, which includes UBBoardPaletteManager (why? seems not to be necessary), which again includes UBPageNavigationWidget, which finally includes this file. Also UBPodcastController includes this file through some chain.
• [x] Rename UBThumbnailProxyWidget.
  It is no longer a proxy, and it is not a widget. New name could be UBThumbnailPixmapItem. Also UBDraggableThumbnailView is misleading (no view) and could better be UBDraggableLivePixmapItem or UBDraggableThumbnailItem.
• [ ] Create a list of unused classes around thumbnails and their inheritance structure.
  Use this list as a base to decide what to keep. It would also be an option to move unrelated parts of the code to another source file to better represent what belongs together due to relations or inheritance.
• [ ] Check pixmap rendering on HiDPI displays.
  It might be necessary to add some handling of the devicePixelRatio, i.e. to set the devicePixelRatio when allocating the pixmap and multiply the size with this factor.

[kaamui]

• Break include hierarchy.UBThumbnailWidget.h is something very special, but due to the current include hierarchy it is even included by the completely unrelated UBSvgSubsetAdaptor, which includes UBBoardPaletteManager (why? seems not to be necessary), which again includes UBPageNavigationWidget, which finally includes this file. Also UBPodcastController includes this file through some chain.

This is particularly annoying. Ultimately each class should be on a separate file.

[letsfindaway]

• Break include hierarchy.UBThumbnailWidget.h is something very special, but due to the current include hierarchy it is even included by the completely unrelated UBSvgSubsetAdaptor, which includes UBBoardPaletteManager (why? seems not to be necessary), which again includes UBPageNavigationWidget, which finally includes this file. Also UBPodcastController includes this file through some chain.

This is particularly annoying. Ultimately each class should be on a separate file.

Or at least each column on the diagram I made above, i.e. each inheritance hierarchy tree. Especially if the intermediate classes are not used outside of that hierarchy.

[kaamui]

I don't think it's enough or that it would be a good practice in all cases. My pov is that until something prevents it, each class should be on its own file. It would help to envision the architecture of OpenBoard. Look at UBDocumentController.cpp for example... I find it really hard to read.

By inheritance, or "topic-related", it would make in both cases some files contain thousands of lines, and I don't see what it provides (on the positive side) for it to be useful. Especially concerning the more "include what you use" approach that has grown the last decade almost everywhere, since C++11.

[kaamui]

• Rename UBThumbnailProxyWidget.It is no longer a proxy, and it is not a widget. New name could be UBThumbnailPixmapItem. Also UBDraggableThumbnailView is misleading (no view) and could better be UBDraggableLivePixmapItem or UBDraggableThumbnailItem.

We could also rename UBDocumentProxy. I don't understand why it is not simply called UBDocument. I think we also lack a UBDocumentsManager class (currently, it is somehow UBDocumentTreeModel), but it's more than that, in fact. My dream is to redesign the entire application with SOLID principles in mind. I think a lot of things could be much simpler.

And yes, in your implementation, they should definitely be renamed to reflect the new architecture

[kaamui]

I quickly tested your last commits. The behavior without PDF seems really good.

With PDFs, sometimes I have a big latency when I scroll through pages, the "Processing..." doesn't appear, the thumbnails remain completely white (during ~10-20s), and then suddenly all the PDFs pages appear in the different visible thumbnails. Did you experience it too ?

I also had a big FPS drop when I was drawing outside the grey rectangle (don't know if relevant), but I didn't manage to reproduce it so it could have nothing to do with what you did, as I have plenty other things running in parallel. I did reproduce it. I'll try to find a reproduction scenario

[kaamui]

Somehow, the behavior that you have with PDF portrait-oriented is better. The control and display screen show the whole scene, but not the thumbnail, so .... maybe all the thumbnails should have the size of the scene, no matter the size of the initial imported PDF.

I'm not quite sure, I'll try to discuss it with the team. Don't put too much effort in the "portrait" question for now.

PS: a quick reminder, seeing all the efforts you put in this rework, that it's not guaranteed it hits the official repo, if there are counterparts that are judged too big here internally. As I already told you, live thumbnails feature has not been asked by our end users, and a lot of them simply don't use this left palette. The team is not particularly interested in, so if it starts becoming a huge thing, they (and I) will probably consider (at least for 1.7.0) that it's not worth the effort.

edit: also, don't you think we should still create a Qt-bug, to report this particular case (i.e a two views with a different size produce the issue where two views without don't, if I understood well) ?

[letsfindaway]

I quickly tested your last commits. The behavior without PDF seems really good.

Thanks!

With PDFs, sometimes I have a big latency when I scroll through pages, the "Processing..." doesn't appear, the thumbnails remain completely white (during ~10-20s), and then suddenly all the PDFs pages appear in the different visible thumbnails. Did you experience it too ?

Yes, this will be part of "Tell thumbnails whether they are exposed on the view"

I also had a big FPS drop when I was drawing outside the grey rectangle (don't know if relevant), ~but I didn't manage to reproduce it so it could have nothing to do with what you did, as I have plenty other things running in parallel.~ I did reproduce it. I'll try to find a reproduction scenario

Please report if it can be reproduced.

[letsfindaway]

Somehow, the behavior that you have with PDF portrait-oriented is better. The control and display screen show the whole scene, but not the thumbnail, so .... maybe all the thumbnails should have the size of the scene, no matter the size of the initial imported PDF.

I'm not quite sure, I'll try to discuss it with the team. Don't put too much effort in the "portrait" question for now.

For me the current behavior looks quite good and usable, too. I will leave it as it is now.

PS: a quick reminder, seeing all the efforts you put in this rework, that it's not guaranteed it hits the official repo, if there are counterparts that are judged too big here internally. As I already told you, live thumbnails feature has not been asked by our end users, and a lot of them simply don't use this left palette. The team is not particularly interested in, so if it starts becoming a huge thing, they (and I) will probably consider (at least for 1.7.0) that it's not worth the effort.

Recalling the original title of this issue: The choice is either to revert live thumbnails or to implement them in a way that it does not disturb the scenes. I will continue here. Then you have these to options to choose. It's up to you;)

edit: also, don't you think we should still create a Qt-bug, to report this particular case (i.e a two views with a different size produce the issue where two views without don't, if I understood well) ?

I don't know what causes this dropdown problem and therefore also not create a QTBUG.

[kaamui]

Recalling the original title of this issue: The choice is either to revert live thumbnails or to implement them in a way that it does not disturb the scenes. I will continue here. Then you have these two options to choose. It's up to you;)

Cool ! Thank you for your understanding.

I don't know what causes this dropdown problem and therefore also not create a QTBUG.

If it's not what I think it is, still the fact that the display view itself doesn't disturb dropdown positioning is telling us something.

[letsfindaway]

Implementation and features are now complete, I think, and it works quite well for me. What I have however observed:

When zooming out very much and then drawing at an area far from the nominal size, the line is not drawn continuously and is only completed once the mouse button is released.

I think this is a fundamental performance drawback of the current approach. As soon as something is connected to the changed() signal, the QGraphicsScene makes a lot more processing. There is some code in QGraphicsScene starting with

    // Ensure all views are connected if anything is connected. This disables
    // the optimization that items send updates directly to the views, but it
    // needs to happen in order to keep compatibility with the behavior from
    // Qt 4.4 and backward.
    if (isSignalConnected(changedSignalIndex)) {

and this code is executed when we connect to the changed() signal. So we fall back to a mechanism of Qt 4.4 and before when we connect to this signal. I can see the effect even if I just write return; in the connected slot. So the performance is not with rendering the pixmap, but with producing the signal. I need a good idea here ;)

[kaamui]

When zooming out very much and then drawing at an area far from the nominal size, the line is not drawn continuously and is only completed once the mouse button is released.

Exactly what I observed twice, without zooming out, after a portrait-oriented PDF import, some drawings, PDF export, doing other things outside OpenBoard, coming back to OpenBoard, drawing outside the nominal size

italic = maybe not needed in the reproduction steps

[letsfindaway]

I think I now have a solution without these drawbacks. What I did:

As said before, I cannot connect to the changed() signal due to the performance drawbacks. I therefore tried to look on the other side of update processing: painting. At least for the active scene we have these paint events, which also carry the repainted region. So I added UBBoardView::paintEvent to catch these events. Here I have to filter those events which are behind the left palette, because this else leads to an endless loop of events. The UBBoardView then emits a painted signal with the affected region in scene coordinates.

The UBBoardThumbnailView connects to this signal and forwards it to the active scene's thumbnail.

The thumbnail handles this signal as it did before for the changed() signal.

This however only updates the active scene's thumbnail. We also need some updating for the other exposed thumbnails. For those I added a timer based approach: whenever a thumbnail gets exposed, I start a cyclic timer firing 10 times in 300ms interval. This nicely covers the time for rendering a background PDF. So when you scroll through the thumbnails, newly exposed thumbnails get updated within a short interval. After that burst, the timer is stopped, so we have no unnecessary background activity.

When looking at the CPU load I now cannot see any increase due to the live thumbnails. Also the problems we sometimes had before when drawing a line are no longer present.

Note: Before creating a PR I would squash the commits into three:

• one doing the actual work,
• one for breaking the include hierarchy,
• and a third renaming the classes.

Is this ok for you?

[kaamui]

Yep !

[letsfindaway]

closed as related PR was merged

[kaamui]

I have a little feedback ;)

if you move a widget without selecting it, the dropdown list position is not updated :)

[letsfindaway]

I have a little feedback ;)

if you move a widget without selecting it, the dropdown list position is not updated :)

Ok, I see. I can easily reproduce this by scrolling the board using the mouse wheel or the hand tool. Seems that the workaround from 2faf67f3423aa991676dbdd7d8f4b95a60470103 has to be applied in this situation, too.

I will see whether I can find a common entry point for such changes of the widget position.

I created #120 for follow-up on this topic.

[letsfindaway]

Implementation and features are now complete, I think, and it works quite well for me. What I have however observed:

When zooming out very much and then drawing at an area far from the nominal size, the line is not drawn continuously and is only completed once the mouse button is released.

I think this is a fundamental performance drawback of the current approach. As soon as something is connected to the changed() signal, the QGraphicsScene makes a lot more processing. There is some code in QGraphicsScene starting with

    // Ensure all views are connected if anything is connected. This disables
    // the optimization that items send updates directly to the views, but it
    // needs to happen in order to keep compatibility with the behavior from
    // Qt 4.4 and backward.
    if (isSignalConnected(changedSignalIndex)) {

and this code is executed when we connect to the changed() signal. So we fall back to a mechanism of Qt 4.4 and before when we connect to this signal. I can see the effect even if I just write return; in the connected slot. So the performance is not with rendering the pixmap, but with producing the signal. I need a good idea here ;)

Note that the same mechanism is used in podcast recording: https://github.com/letsfindaway/OpenBoard/blob/9de37af2df1a7c0d88f71c94ab2db1815d082862/src/podcast/UBPodcastController.cpp#L484-L485 This will also introduce this performance drop for podcasts. @kaamui : If you think it is worth changing the mechanism also here, please open an issue here.