Bug 163457 - Display of Comments erratic
Summary: Display of Comments erratic
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Calc (show other bugs)
Version:
(earliest affected)
25.2.0.0 alpha0+
Hardware: All Linux (All)
: medium normal
Assignee: Armin Le Grand (allotropia)
URL:
Whiteboard: target:25.2.0
Keywords: bibisected, bisected, regression
Depends on:
Blocks: CairoSDPR
  Show dependency treegraph
 
Reported: 2024-10-16 06:27 UTC by Elmar
Modified: 2024-11-04 14:25 UTC (History)
4 users (show)

See Also:
Crash report or crash signature:


Attachments
sample file (44.57 KB, application/vnd.oasis.opendocument.spreadsheet)
2024-10-16 06:30 UTC, Elmar
Details
Load, hover over commented cell (0,0) and cell bottom-right to trigger comment paints (8.10 KB, application/vnd.oasis.opendocument.spreadsheet)
2024-10-24 09:38 UTC, Armin Le Grand (allotropia)
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Elmar 2024-10-16 06:27:57 UTC
Description:
When I create a note, it does not display on mouse hover. After saving & opening the doc, it display once, then not again.
The corner marker is so small that can barely see it (on 15" laptop screen)

Steps to Reproduce:
1.create Comment
2.hover on cell / select cell
3.save and close doc
4.open doc
5.hover
6.move mouse pointer
7.move back

Actual Results:
does not show Comment
after save and reopen, shows cropped comment once then not again
is a regression - does not doe this with earlier v25 and not in v24.2
corner marker is too small

Expected Results:
behaviour / corner marker should be as in
Version: 24.2.6.2 (X86_64) / LibreOffice Community
Build ID: 420(Build:2)
CPU threads: 8; OS: Linux 6.8; UI render: default; VCL: gtk3
Locale: en-GB (en_GB.UTF-8); UI: en-US
Ubuntu package version: 4:24.2.6-0ubuntu0.24.04.1
Calc: threaded


Reproducible: Always


User Profile Reset: No

Additional Info:
[Information automatically included from LibreOffice]
Locale: en-US
Module: SpreadsheetDocument
[Information guessed from browser]
OS: Linux (All)
OS is 64bit: yes

Version: 25.2.0.0.alpha0+ (X86_64) / LibreOffice Community
Build ID: dfeefe2e97a5412a445a3a508acb2d9ae05138e5
CPU threads: 8; OS: Linux 6.8; UI render: default; VCL: gtk3
Locale: en-GB (en_GB.UTF-8); UI: en-US
Calc: threaded
Comment 1 Elmar 2024-10-16 06:30:23 UTC
Created attachment 197071 [details]
sample file
Comment 2 raal 2024-10-16 14:46:46 UTC
Confirm with 25.2, Linux. Regression
Comment 3 raal 2024-10-16 14:53:44 UTC
This seems to have begun at the below commit in bibisect repository/OS linux-64-25.2.
Adding Cc: to Armin Le Grand ; Could you possibly take a look at this one?
Thanks
 7813d2b047401f6212870e4ea557fa2a059f342e is the first bad commit
commit 7813d2b047401f6212870e4ea557fa2a059f342e
Author: Jenkins Build User <tdf@maggie.tdf>
Date:   Tue Aug 27 13:21:18 2024 +0200

    source bf3cd8e50f886084500e3a8ff72ff4ffab05ddd7

172394: CairoSDPR: Make using SDPR dependent of ExperimentalMode | https://gerrit.libreoffice.org/c/core/+/172394
Comment 4 Armin Le Grand (allotropia) 2024-10-21 09:52:23 UTC
@raal: the bibisect from comment 3 cannot be correct, it did not activate the CairoSDPR, activation was still dependent of activating experimental mode. I also doubt from the description that it has anything to do with CairoSDPR rendering, so should probably not be member of tdf#163129. Can you have another loo, please...?
Comment 5 raal 2024-10-21 17:14:31 UTC
(In reply to Armin Le Grand from comment #4)
> @raal: the bibisect from comment 3 cannot be correct, it did not activate
> the CairoSDPR, activation was still dependent of activating experimental
> mode. I also doubt from the description that it has anything to do with
> CairoSDPR rendering, so should probably not be member of tdf#163129. Can you
> have another loo, please...?

Hi Armin,
retested with following results:
git checkout 7813d2b047401f6212870e4ea557fa2a059f342e - bug is here (experimental enabled)
git checkout HEAD~1 - bug is not here (experimental enabled)
git checkout 7813d2b047401f6212870e4ea557fa2a059f342e - bug is not here (experimental disabled)
git checkout master - bug is here (experimental disabled)

but new bisection with turn off experimental mode: 7330079faa9a5bbf05f292a23318c5102b95dc5c is the first bad commit
commit 7330079faa9a5bbf05f292a23318c5102b95dc5c
Author: Jenkins Build User <tdf@maggie.tdf>
Date:   Mon Sep 23 14:20:13 2024 +0200

    source 1acd37a671b9d3633a7d31a0b60478815fbc685f

173330: CairoSDPR: Activate globally to check builds/tests | https://gerrit.libreoffice.org/c/core/+/173330

It really looks that CairoSDPR is culprit ..
Comment 6 Armin Le Grand (allotropia) 2024-10-23 13:27:36 UTC
(In reply to raal from comment #5)
It's much easier to just use EnvVar
    DISABLE_SYSTEM_DEPENDENT_PRIMITIVE_RENDERER=1
on the same version - it disables CairoSDPR. Thus - to check if ssth happens due to CairoSDPR - just disable it and have a look. HTH!
Comment 7 Armin Le Grand (allotropia) 2024-10-23 13:33:31 UTC
Used DISABLE_SYSTEM_DEPENDENT_PRIMITIVE_RENDERER on master and indeed happens as described. I have no idea how that might happen, but will have a look
Comment 8 Armin Le Grand (allotropia) 2024-10-23 14:39:26 UTC
It has to do with OuputDevice's offset/clipping stuff. It starts at ScNoteMarker::Draw() uses lcl_DrawWin in sc/source/ui/view/notemark.cxx to call SingleObjectPainter for the draw object used for the comment. From there ObjectContactOfObjListPainter::ProcessDisplay uses createProcessor2DFromOutputDevice which creates a CairoPixelProcessor2D with nOffsetPixelX/nOffsetPixelY != 0.
This means a non-real ('virtual') window is used in VCL/OutputDevice which the CairoPixelProcessor2D::CairoPixelProcessor2D covers using cairo_surface_create_for_rectangle. This works e.g. for the Impress template dialog (I made a fix there) but seems not to be complete for this paint to happen as needed.
Need to check that...
Comment 9 Armin Le Grand (allotropia) 2024-10-24 09:36:40 UTC
This is *very* strange. I debugged into it, but all seems to work well, except the output of the result. Then I tried to paint something directly to the involved OutputDevice to visualize the dimensions/areas where the target stuff gets *not* painted - and voila, that *gets* painted, and also the wanted stuff using the CairoPixelProcessor2D afterwards (!)

This hints to something not being initialized in the VCLCairoBackend for that OutputDevice, UNITL something gets painted. I used (diff):

-----<snip>----
diff --git a/drawinglayer/source/processor2d/processor2dtools.cxx b/drawinglayer/source/processor2d/processor2dtools.cxx
index f87bffea6b21..e53d04eb0677 100644
--- a/drawinglayer/source/processor2d/processor2dtools.cxx
+++ b/drawinglayer/source/processor2d/processor2dtools.cxx
@@ -118,6 +118,13 @@ std::unique_ptr<BaseProcessor2D> createPixelProcessor2DFromOutputDevice(
         // let the CairoPixelProcessor2D do this, it has internal,
         // system-specific possibilities to do that in an elegant and
         // efficient way (using cairo_surface_create_for_rectangle).
+
+        if (0 != rTargetOutDev.GetOutOffXPixel() || 0 != rTargetOutDev.GetOutOffYPixel())
+        {
+            rTargetOutDev.SetLineColor(); // rTargetOutDev.SetFillColor();
+            rTargetOutDev.DrawTransparent(tools::PolyPolygon(tools::Rectangle(Point(0, 0),rTargetOutDev.GetOutputSize())), 95);
+        }
+
         std::unique_ptr<CairoPixelProcessor2D> aRetval(
             std::make_unique<CairoPixelProcessor2D>(
                 rViewInformation2D, static_cast<cairo_surface_t*>(aData.pSurface),
-----<snip>----

NOTE: The stuff we WANT to paint only gets painted in the area where the DrawTransparent *does* paint something (!), so if you use
         rTargetOutDev.DrawTransparent(tools::PolyPolygon(tools::Rectangle(Point(500, 500),Size(2000, 2000))), 95);

the paint using the created CairoPixelProcessor2D *only* paints in that area (!)
Comment 10 Armin Le Grand (allotropia) 2024-10-24 09:38:18 UTC
Created attachment 197223 [details]
Load, hover over commented cell (0,0) and cell bottom-right to trigger comment paints
Comment 11 Armin Le Grand (allotropia) 2024-10-24 09:47:41 UTC
@Caolan: need help here. Please read comment 9. I create a CairoPixelProcessor2D based on the cairo_surface_t that I get from an OutputDevice using GetSystemGfxData, then extracting SystemGraphicsData.pSurface. The CairoPixelProcessor2D creates a cairo_t context and uses it.

I have a case here where all ptrs/objs get created, but output fails. It seems as if something in the OutputDevice and thus in the VCLCairoBackend is not initialized.

It *gets* initialized when I paint something to it, see patch in comment 9. What in that patch makes it initialized, and - very strange - ONLY to the area that test-paint does paint to...??

May also have to do with that OutputDevice having GetOutOffXPixel/GetOutOffYPixel set ('virtual' OutDev with internal offset).

May also have to do with that OutputDevice being a Window (not sure yet), thus refresh/DoubleBuffer of window. Using the CairoPixelProcessor2D may make the Window think that *noting* was painted...?
Comment 12 Armin Le Grand (allotropia) 2024-10-24 11:13:07 UTC
Update: It is a OUTDEV_WINDOW...
Comment 13 Armin Le Grand (allotropia) 2024-10-28 19:26:56 UTC
Have now debugged into it, It seems that CairoCommon (VCLCairoBackend) works with 'damage' being applied when painting, directly to the gtk win stuff, unreachable for anything outside that subsystem.

@Caolan: Make that accessible somehow (at Window?), Alternatives are:

- Do not use CairoSDPR for Window. Looks bad at 1st, but this is up to now the single case where that happens - showing/popping up those comments in Calc should not be a direct paint to a Window, but Objects on the Overlay

- Render using ConvertToBitmapEx and paint BitmapEx to Window. Expensive part is that a BitmapEx will be extracted and then converted to Cairo again to be painted

- Render to a buffer VDev. That would allow to BitBlt rendered content to Window as target, but size of the VDev would probably be Window size, so not effective

- When 'damage' would be available it may also need to be done for the whole Window size.

I would prefer last one, at destruction of a CairoSDPR (thus potentially lots of rendering may happen). All possibilities are - as mentioned - for the rare case of rendering to a Window directly...
Comment 14 Caolán McNamara 2024-10-28 20:01:10 UTC
This "damage" thing is used to tell gtk what parts have changed to get gtk to request them from us. So we inform gtk with gtk_widget_queue_draw_area from GtkSalFrame::damaged which currently gets called from CairoCommon::releaseCairoContext (which takes the bounds of the area changed by rendering)

In that releaseCairoContext we fetch this "damage handler" with cairo_surface_get_user_data on the cairo_surface_t where that is set by cairo_surface_set_user_data in vcl/unx/gtk3/gtkframe.cxx for a surface that backs up a gtk window.

So... if you have a cairo_surface_t SystemGraphicsData.pSurface (and assuming that is the surface that has this user_data set on it) then it might be that the way to go is to use cairo_surface_get_user_data on the surface to fetch that damage handler in order to use that to inform the gtk stuff to refetch from us the changed part of that surface to refresh its window.

That might imply moving DamageHandler and getDamageKey out of vcl/inc/headless/CairoCommon.hxx and into something visible outside vcl, like include/vcl/cairo.hxx to get is usable from outside vcl.

Hope that helps and isn't a misleading idea.
Comment 15 Armin Le Grand (allotropia) 2024-10-29 11:39:32 UTC
Yes, I see, that's what I saw debugging. I experimented, but do not get to a green path. I would at least need to costruct a temp CairoCommon with the cairo_surface_t I have. Not sure how that gtk ptr is then set there (the cairo_surface_get_user_data) - only set on two places in vcl/qt5/QtFrame.cxx resp. vcl/unx/gtk3/gtkframe.cxx, so probably not on my surface/context anyways...

Could we not have something at a VCL Window (only for HEADLESS) like ApplyAllDamage() that only does something when it is gtk/qt5 that I can just call...?
Comment 16 Armin Le Grand (allotropia) 2024-10-29 15:08:21 UTC
@Caolan: Done https://gerrit.libreoffice.org/c/core/+/175794 now, pls comment :-)
Comment 17 Commit Notification 2024-10-31 11:26:15 UTC
Armin Le Grand (allotropia) committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/9ea9cd14ffc69e6597996da09943e1ce11a50d6f

tdf#163457 CairoSDPR: Need to apply 'damage' to gtk

It will be available in 25.2.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.