Found a simple way to do that (after some experimenting),
needed to trigger the Overlay update, and that can just be
done with a Reschedule. Other places that used
flushOverlayManager() checked, but work on their own, so
we do not need to bring that OverlayManager flush back.
Unfortunately this triggers CppunitTest_desktop_lib
"DesktopLOKTest::testRedlineCalc", probably due to not
only the timer triggering but 'other' stuff that better
runs later (...?). Not doing it - as long as we need
to do it - is safer, but forces to keep that flush just
for this single usage.
Change-Id: Iacf18d688597129baba6e893252487d73a1fdfff
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/182834
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
had to refine a typo, also all places with '\{\}\)->' that
call functions at an incarnation have to be made safe, so
added this also to getPrevControlPoint/getNextControlPoint
calls.
see commit 3c82f49fce for more details
Change-Id: Id8c1abfeb41961a9cca85ef28b2ea70beb07f37c
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/182548
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
In Presentation with title/Body with text directly activating
one by clicking makes the other 'blink' briefly when it gets
deactivated. All that has to do with OverlayManager, repaints
and the text being 'hidden' (to not be shown twice during
edit). That needs to be synchronized since OM refreshes using
a timer and direct paint to the target, while repaint uses
system's invalidate-and-paint-callback functionality. It
has shown that the immediate flush of the OM that was used
before is not needed anymore, so I removed that.
Note that there is task tdf#102293 mentioned as reason for
the flush, so I checked that the there mentioned triple/
double-click on text does the right thing.
Change-Id: Id9539129c5d6989959974befd15300abdfef3186
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/181904
Tested-by: Jenkins
Reviewed-by: Thorsten Behrens <thorsten.behrens@allotropia.de>
This is a follow-up to 'Avoid short blinking of text
when activating TextEdit' commit. The update using the
timer works nowadays flawless, so no longer needed.
I separated the two places in two commits because this
one handles some places I do not know good enough to
be completely sure, so it would be simpler to turn
back in case of (unexpected) problems.
Change-Id: I336515838949585b98101ff566e9137b898da82f
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/181906
Reviewed-by: Thorsten Behrens <thorsten.behrens@allotropia.de>
Tested-by: Jenkins
A PixelSnap was used before using GetOutputDevice() and
LogicToPixel/PixelToLogic (what was incorrect, would need
to take care of 1/2 pixel in logic for rounding). We do not
need that anymore, in fact it leads to the text slightly
'jumping' around by up to 1 pixel (of course).
We paint text nowadays using decomposed TextPrimitives
with sub-pixel precision and similar (using a shortcut)
for text in TextEdit on the Overlay, also using sub-pixel
precision. Just remove this to avoid Text being displayed
different in TextEdit and EditView paint visualizations
and assign given value to OutputArea unchanged.
If this should cause problems somewhere (I do not think so,
we try to paint text with Sub-Pixel quality now anyways)
this can also be done in alternative ways and only for
TextEdit on Overlay.
Change-Id: If177ceec880a29a6d102ad4f0dddf891d95b69c2
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/182094
Tested-by: Jenkins
Reviewed-by: Thorsten Behrens <thorsten.behrens@allotropia.de>
In SdrTextObj the methods impDecomposeAutoFitTextPrimitive and
impDecomposeChainedTextPrimitive used the wrong
OutlinerParaObject. Unfortunately there are two OPOs available
in the SdrTextPrimitive2D, one by using
(A) getOutlinerParaObject()
and one using
(B) getSdrText()->GetOutlinerParaObject()
whereby (A) is the one which contains possible changes from
an active text edit while (B) contains the original text at
the start of the possible text edit.
By using (B) those two decomposes were not able to show
the already changed text, thus the update in the SlideShow
seemed not to work.
Change-Id: Id2dcc3924fe8029688d0e76ca0f3f7300f461bd5
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/181490
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
SfxStateCache had problems processing the extra Item
states disabled/invalid. Note that due to not using a
Pool here it is not possible to use an ItemSet or an
ItemHolder, those would automatically handle these
cases correctly.
In this cache, this currently needs to be done
handish. Also note that because of that this may break
again when changes in the Item/ItemSet/ItemHolder
mechanism may be done.
Change-Id: I007bc25c727e6432062fa0d2af8215a912cb2fff
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/175871
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
Tested-by: Jenkins
If a CairoSDPR does directly render to a Window the changes
do not get reliably visualized due to the Cairo backend
and gtk/QT usage being dependent of being told what
'damage' is done to refresh the window accordingly.
The VCLCairoBackend is doing that, but without reach
from the outside.
Note that this is a rare case, in fact the 1st one discovered
where a CairoSDPR is painting to a Window directly, see
comments in the task. Even this occasion should not do
that - showing the Comments in Calc on the Overlay would
be major to just painting to the Window roughly.
There are other possible workarounds (also see comments in
the task), but just adding to be able to set the needed
'damage' in case target is a Window is simplest and
potentially the fastest way to do this.
Since usage of CairoSDPR is bound to use GetSystemGfxData
anyways it is okay to react there and call a method
ApplyFullDamage that exists on Graphics when the flag
USE_HEADLESS_CODE is active. That forwards to the
CairoCommon being responsible for that Window and does
the minimal necessary to apply 'damage' to the full
Window.
This is done when constructing the CairoSDPR, thus
from the timing that incarnation can then paint anything
and it seems that at the next occasion something can
handle the callback from gtk all is painted. If that
should change it may get necessary to separate
the usage of ApplyFullDamage from GetSystemGfxData,
but works as intended for now.
Change-Id: I5442f3413e43418954da29a18d66dea27e25e655
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/175794
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
to no longer make the wrong data appear when a
CF gets changed on one tab. For more info, see
text in task.
Change-Id: I7bf582eb9a2fe59b9a4b3d280639661c3aa7e608
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/175779
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
Tested-by: Jenkins
That was missing with the result that for some cases the
painted text (where it was missing) differed from the
text in EditEngine mode (was set there).
Change-Id: I23ddf8e90be2335c56e5a485308c753dfe72a878
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/175127
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
When the non-native SlideShow in DrawViewShell::Command
in drviews4.cxx (see !bNativeShow) is active, the above
checks/actions have to be done to make the EditView work
normally, but use the "page" standard context menu
fallback only when SlideShow is not running to get the
SlideShow popup menu - as expected when SlideShow is
running.
Change-Id: I6e3bc69023218fc0decce61581ddbc498e00d39c
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/174437
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
Tested-by: Jenkins
Saw that on a stack on gerrit, checking if that is
okay for the rest of functionality, should be
Change-Id: Ifa61a54edb46b0c98afc59b77674e3686fa29a59
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/174311
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
Was not used and - if needed - should be done by embedding
the sequence of Primitives sent to it to a
ModifiedColorPrimitive2D containing the intended
BColorModifier. Keep clases as simple as possible.
Change-Id: I99de18cc971d6b396ad291ef88b59bd7599f9196
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/174314
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
Tested-by: Jenkins
Added general interfaces to be able to render directly
to an RGBA target SDPR:
- processor2d::createPixelProcessor2DFromScratch creates
a target-SDPR with given pixel size and RGB or RGBA,
owning and using a cairo surface internally
- processor2d::extractBitmapExFromBaseProcessor2D extracts
rendered content to BitmapEx, including alpha
All this is currently only implemented for Ciaro, thus
the internal impls are encapsulated by USE_HEADLESS_CODE,
but already created gererally available. The return values
have to be checked to see if an evtl. shortcut is possible.
For convertToBitmapEx this means that it can do conversions
much faster than up to now for cairo when CairoSDPR is
available, else it has to to the older slower way that
also works and is the default: Create RGB content, create
Mask, RemoveBlendedStartColor (see convertToBitmapEx
implementation). This works and has the same quality,
but needs two renderings and one bitmap operation, thus
is clearly slower.
Note that these interfaces can and will be supported for
other SDPR implementations in the future, thus will make
this converter automatically faster on systems where we
will have a SDPR in the future, too.
Also note that this is the gereral converter from
a sequence of Primitives to Bitmap data, already used in
many places, inculding UNO API, thus is expected to have
some impact on conversion efficiency - if Cairo is used,
so e.g. also headless.
Change-Id: Ia638a549a04b19622892d91651317ec6727b6cd0
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/174266
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
Tested-by: Jenkins
I have stumbled over the GDB integration in vscode hanging
when there is a BP and *below* a declaration of e.g. a
basegfx::B2DPolygon. This seems to happen due to the
instance not yet being incarnated, but the variable view
already listing it and triggering methods in basegfx.py
to evalualte stuff.
It just hangs/loops, I guess it's fetching the PointCount
from a random mem ptr'ed Polygon, then tries to evaluate
something with the Points.
I do not really know about what to do here, but I figured
out that testing 'if self.exists()' where the instance
gets accessed (using gdb.parse_and_eval) helps.
Thus this solves the problem for me and - after stepping
over the incarnation of the object I can see it's true
content.
I hope someone who might have more knowledge about that
GDB and python stuff can have a look and maybe tell if
tis is OK or should be done in a different way.
Change-Id: Ia844b0aded97220df7f4605fdcffeac5e2270db3
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/173826
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
Tested-by: Jenkins
The order of parameters for vcl::bitmap::premultiply and
vcl::bitmap::unpremultiply was wrong because when doing
that I just used the same order that the indexing that was
to be replaced used it. Corrected that.
Change-Id: I59473acca8afdc58105b3381d1fdcedd6c0fd83b
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/173683
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
Reviewed-by: Caolán McNamara <caolan.mcnamara@collabora.com>
Tested-by: Jenkins
I formally tried to avoid un/re-register if Item needs
surrogate support, but there are more aspects to this,
e.g. the pool could be different or even the WhichID
of the Item, so just do the necessary here. These cases
are also not too often used.
Change-Id: I8d1e8854f5ce3cac5566c076797162ba2e4b4b75
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/173329
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
This one still used just a hash to feed a unordered_map
what is not safe. Changed to unordered_set and added
hash and equal operators.
Change-Id: I2a426c449536747ad59f8bd8781a7eadceea9183
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/170417
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
Up to now that genereal buffering mechanism used
typeid.*hash_code to identify the class. As we
have learned this is not safe. Thus I changed it
to use enum'ed SystemDependentDataType definitions
Change-Id: I803912ec419290db1d40bae0bc41364fad64cbfd
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/170385
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
for the ItemInstanceManager the identification was done
using typeid/hash_code, but that has problems - it is
only defined to be identical for two instances of the
same type, but other types can have the same code.
Thanks to Noel to find that out, that is not reliable
to be used.
In the meantime we have SfxItemType to identify Item
instances, so I changed the code to use that.
The master had five additionally added Items that
use the (2a) method of this mechanism (see comments
in svl/source/items/globalpool.cxx for that). The
gloal is to use as less as possible of this Items in
that mechanism, so I checked.
For four of these hashing was used, so the mem/runtime
aspect is okay -> access is fast enough to prefer
mem over runtime. Adding hashed Items (or any other
fast mechanism) is always okay.
One did not have that (SvxBoxItem) and used just the
fallback ItemInstanceManager, so I removed it. We
now have 18 Items for which that mechanism is
initialized immediately.
Noel also already moved the countdown for starting
to use the mechanism in case of default handling
(case (1), NUMBER_OF_UNSHARED_INSTANCES) to the
unorderd_set. someting I had already planned to do,
too - thanks!
Change-Id: Icf6f427e5ea64672f385357aaad75bb5b7fcbf98
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/170314
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
There is the complete BColorModifierStack support for
primitives for Bitmaps, e.g. hue/saturation, etc, but
it was slow due to not being buffered, so had to be
re-created often. I changed this to use the common
buffering mechanism to improve this.
Up to now a fallback to use the old Graphic
manipulators for that purpose was in place since this
was faster, but had to be done every time. I have now
changed the priority to using the primitive way to
handle things, but kept the fallback code - just in
case.
Note that the new stuff is faster, but even much faster
when the bitmap is copied and appears multiple times ->
the same buffered instance is used, and SDPRs then use
the system-dependent data buffered at that prepared
data.
Also note that this change does not only speedup
CairoSDPR, but all PrimitiveRenderers, including the
VCL and Metafile ones. In principle everything that
uses BitmapEx::ModifyBitmapEx.
Had a 2nd thought: Only the content Bitmap gets changed,
so for this case we do not need AssociatedAlpha and
watch for it to not have changed. Removed that.
Change-Id: I2ee36cc84bdc1c723aa01f872edbfd1f51e11c2d
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/170305
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
Avoid decomposition of it in AnimatedExtractingProcessor2D
by handling there directly. Goal is to find/extract animation
primitives, so only do something when the contained Graphic
is active
Change-Id: I1d168428ddbaaac2c9d5fde7b26be380ba442c30
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/170203
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
With all the changes done for Items we can now do deeper
basic changes to the ItemSet itself with manageable risk.
I already did https://gerrit.libreoffice.org/c/core/+/166455
aka 'ITEM: Add measurements for SfxItemSet usages' to
get some statistical information about the fill/usage grade
of the ItemSet's fixed PtrArray to SfxPoolItems, check
that out to get an own picture.
Those results show that an average usage is between some
extremes ranging from 0% to 50%, but when using more checks
and using multiple files/interactions/edits in all
applications we end up with around typical 12%-19% of that
array used, the rest is nullptr's.
Thus I thought about one of the initial ideas of this
series of changes (ITEM), to use a std::unordered_map
(A) instead of that fixed array of SfxPoolItem Ptr's (B).
Tthat again was for a complete type-based rewrite, which
I once did a POC, but the code cannot be adapted to that,
just too much work.
Those are very different in architecture, (B) is done
since a long time (since ever), but as pointed out above,
(A) is now possible. There are many aspects to it, let's
grep some:
Speed (iterate): (A) and (B) are both linear. (A) has
less entries, but may touch different mem areas
(buckets). (B) is linear, but many empty spaces which
are usually uselessly iterated.
Speed (access Item by WhichID): (A) is hashed by
WhichID, so mostly linear for unordered_set/hash.
(B) is in a linear array, but has to calculate the
offset for each WhichID access.
So I guess speed will mostly equal out.
Memory: (A) will be dynamically allocated (buckets),
but stl is highly optimized and may even re-use areas,
has to provide some extra info but will need less
places for Items since it's dynamic and can start empty.
(B) will be allocated once (except AllItemSet) and may
even be 'derived' to the ItemSet (SfxItemSetFixed), but
has to allocate all space at once.
I can go in lots of more detail here, but due to the
results of the statistics I just made a test now,
including measuring some results (will include in
gerrit, not here). I used two pro versions for that.
That way I have some data now.
Result is:
- It is now possible to do this, it runs stable :-)
- Speed: As expected, mostly no change
- Memory: Depending on usage, 0% to 25% less,
usually around 8-10%
Side effects:
- SfxAllItemSet could be done without WhichRanges,
thus without expensive 'merges'
- SfxItemSetFixed is not needed. While the idea is
good, it needs a lot of extra stuff
- Access to Items is linear if set
- WhichRanges: Still needed, but for vaildity
checking/filtering of ItemSet content
- WhichRanges: Worth to think about if these are
needed at all, probably just exist for historical
reasons since allocation/number of added Items
was never ever dynamic -> just not allocatable
Putting the current version on gerrit, may still
trigger some UTs (checked SW/SC/SD...)
I did not like that functionality at ItemSet that
hands out a vector of the set items for cases where
to avoid iterating and deleting items at the same
time at an ItemSet, so changed to using a local
vector of remembered WhichIDs and deleting after
the iterator is done. I also saw some strange
usages of SfxItemIter in sw which i will have to
check.
Since there are still problems with UTs which I can
not reproduce locally I have now added asserts to
the case that an ItemSet gets changed with still
having active SfxItemIter(s). That is always an
error, but with the architecture of that fixed
array of ItemPtrs did not have devastating effects
(purely by coincidence).
With that asserts, UTs run well in SC and SD, but
in SW I get 11 (eleven!) asserts from the UTs, all
of them from 'ITEM: SfxItemSet ClearItem' BTW.
I guess these have to be fixed before thinking
about this change...
Good news is that all 11 cases were the same in SW,
in SwHistorySetAttrSet::SwHistorySetAttrSet which
does some strange things using two SfxItemIter in
parallel. Thus SW UTs are also clear and I see no
more errors caused by ItemSets being changed while
SfxItemIters are alive.
Bad news is that I still have errors to hunt...
NOTE: Have now cleaned all UTs, this showed that
there are some unexpected side-effects of the Items
being processed in another order when SfxItemIter
is used, also found one case where a
WhichRangesContainer is constructed for a
SfxItemSet using the set items from another ItemSet
and SfxItemIter to do so. There *might* be more
cases not covered by UTs.
NOTE: While speed stays the same and mem is reduced
up to 25% (see measurements in 1st comment) another
*important* aspect is that this frees the way for
using ItemSets *without* WhichRanges - these are
necessary mainly to create that fixed array of
pointers to the Items in a *manageable* size. With
a dynamic structure like unordered_set there is
in principle *no need* anymore to use WhichRanges
to pre-define the Items a Set could hold. There
is one exception: We have cases where one ItemSet
is set at another one with defined WhichRanges to
*filter* the contained Items - these would have to
be identified. This is a rare case and we would have
to check cases where an ItemSet gets set at another
ItemSet. This would be as if all ItemSets would be
AllItemSets in principle - much easier for everyone.
NOTE: Waited for 24.8 split just to not take
unnecessary risks.
Change-Id: I75b0ac1d8a4495a3ee93c1117bdf618702785990
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/166972
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
Tested-by: Jenkins
In the case the input file does have a graphical
representation of the Diagram as image file, the
DiagramModel was not fully loaded. For being able
to edit the Diagram in the future it is necessary
to re-create the geometry, thus a full model is
needed.
Needed to know at recreation if this is the first
time the geometry gets self-created using the
layout/creation mechanism to decide if to apply
the style or use the saved/reapplyable data from
secure/restoreDataFromShapeToModelAfterDiagramImport
mechanism, added that.
Change-Id: Icb7590306ab59728e83b800b8637333e0d372de5
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/168223
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
Tested-by: Jenkins
I was wondering how much of that arrays of pointers
to SfxPoolItems is actually used in the SfxItemSets in
real office runtime, so I added code now to measure that.
It does use the state of the ItemSet at destruction,
so it is possible that items were added/removed which
are not covered, but most cases of internal usages do
not do that.
I then check/sort the collected data at office shutdown,
it will be printed as SAL_INFO when svl.items/vcl.items
is set, so use this to see the data.
This gives info about the average space utilization in
different ItemSets with different sizes, also gives an
insight about used ItemSet sizes and amount of their
usages.
Of course results differ from app to app and dependent
of what is done with the office, but interestingly when
using quite some files opening from all apps it
toggles/normalizes to something around 20-25%...
Change-Id: I3eb2f0401c39a5bdb5d1d8176e95df07be4c111a
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/166455
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
Tested-by: Jenkins
There is a mechanism to pre-fetch the next slide, but
at the last slide that just gets not reset/refreshed.
Thus the last slide *is* the buffered slide.
When making changes and refreshing the SlideShow display
for the last slide this pe-fetched one is just used, so
the slide content gets not re-created and thus not
updated.
I added needed code to do that now when reacing the last
slide to make that work.
Change-Id: Ie57e6917e0996d5fce46021aeb933064c47dd90f
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/166011
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
The support for MouseWheel was missing. That showed
when using it in IASS the scroll commands were always
feeded to the SlideShow. Thus it was not possible
to e.g. scroll using MouseWheel in EditView when
IASS was active and SlideShow running.
This happens in ViewShell::HandleScrollCommand. It
needs to be made focus-dependent when IASS is active.
This is the same as already done for keyboard input,
so I consolidated this now to a method called
ViewShell::useInputForSlideShow() that does all the
necessary stuff and answers as needed. Using that
in various places: keyboard and MouseWheel, but also
adapted other places where the same has to happen,
mainly some gesture stuff.
Change-Id: I1a697e4b35b195695f1a5ea2305a3cee8851fa8d
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/165929
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
It looked like in OutlinerMode in IASS when doing
changes updating the other views were missing. After
debugging and finding no error I found out that the
text as COL_AUTO is painted white on white - all
updates happen but are invisible - argh.
After some more debugging I found that in
ViewShellBase::GetColorConfigColor only the
DrawViewShell case was handled, so I added the
OutlineViewShell now. Since that ViewShell has
no SdViewOptions I hard-coded the DOCCOLOR
to COL_WHITE.
That method returns {} aka COL_BLACK as default
which is a bad default for an office package
with paper as target, so I also changed that
to COL_WHITE - which is the default for unknown
ViewShells now that way. Also adapted the warning
to mention an 'unknown ViewShell' now.
Change-Id: I580a151b4c0a9eb46d190ba84b0c6d0798dc21d7
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/165907
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
When being in IASS mode and adding e.g. an effect to
a Shape (or anything else that triggers a preview)
and the PresenterConsole being active, we got a
crash. That happend due to the PresenterConsole
being initialized *twice* due to interpreting that
preview as SlideShow start. It also closed due to
interpreting the preview end as SlideShow end. The
next SlideShow end then bites the dust in an already
messed up situation.
To solve this, SlideshowImpl::startShowImpl now only
uses NotifyDocumentEvent "OnStartPresentation" when
this is not a SlideShow startup.
I also secured PresenterScreen using a local bool
to remember if it is initialized to avoid when that
would be done twice (also used for shutdown).
Change-Id: Ice588e0783fd39ec46d90a40affcaf2f789df8ec
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/165356
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
I replaced that SfxPoolItem* in SearchAttrInfo with
a much safer SfxPoolItemHolder and adapted and
simplified used code - it does not need to take
care of Item lifetime/Cloning itself. That works
well.
I did not find out why that 'invalid' state is used
in the SrchAttrInfoList, but seems to be needed.
Thus I keep this for now as it is (it can be all
expressed/used using SfxPoolItemHolder, too).
Change-Id: I4b769f43128cb2e25153919f7652b2f032393123
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/165167
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
I checked if this is used, but it can be replaced
using Clear() -> all. This prevents that the whole
array of Items in an ItemSet gets set to
INVALID_POOL_ITEM.
I also checked if INVALID_POOL_ITEM/IsInvalidItem
is needed at all representing SfxItemState::DONTCARE
but it is and still will need to be set for individual
Items.
At last checked if SfxItemState::UNKNOWN and
::DISABLED really need to be separate states, but
indeed there are some rare cases that need that.
To make things more consistent I also renamed
SfxItemState::DONTCARE to SfxItemState::INVALID
to better match Set/IsInvalid calls at ItemSet.
The build showed a missing UT and led to a problem
due to the hand-made ItemSet-like SearchAttrItemList.
The state 'invalid' seems to be used as 'unused'
marker. It should be changed to use SfxPoolItemHolder
and not need that. For now, set by using an own loop
to set to that state.
Change-Id: Ifc51aad60570569a1e37d3084a5e307eed47d06c
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/165035
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
This was missing in the mechanism to suppress
EditView exclusive content, so it is now also
suppressed in PDF export.
Also changed TextHierarchyEditPrimitive2D to
also decide inside it's local get2DDecomposition
implementation, so it does not need to be#
supported in PrimitiveProcessors (and cannot be
forgotten in future implementations).
Also added support for EditView exclusive
Primitive's name in idToString (missed that).
Change-Id: I61efc9dddb7d6f0190c7b1fb4b0acf98ab5767f5
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/165109
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
We have the case that e.g. a placeholder Graphic/Text
shall only be processed/shown in the EditView of the
application, but not in any other Primitive-based
usages, including exports, SlideSHow, etc.
I already thought about that when initially converting
Draw/Impress to Primitives, but always found other
solutions, mainly because full XShapes/SdrObjects
had to be taken into account, that can be handled in
VC/VOC/OC implementation. But here we have varying
visualisations of one shape.
Thus I implemented a helper Primitive called
ExclusiveEditViewPrimitive2D. Anything you embed
there will only be processed when the switch to
do so is active in the ViewInformation2D. And
that switch is only set in the EditView paint.
Change-Id: I307f4b0fe7f8faf98789787f216cac7be86a0515
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/165066
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
Method validRanges2() was encapsulated by DBG_UTIL
but used inside assert, so adapted the define
Change-Id: I695a0c85f389d0c37b24e6e1986037ce6fb0c805
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/165052
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
This was not working for multiple Windows for
a document, and also not supported for SlideShow
until now.
If you edit a Text on a Slide (Object, PresObj,
...) that text is not yet set at the Model until
the TextEdit ends. In that situation those changes
are now propagated to other views visualizing that
object. This is done with slight slowdown to not
do it all the time while typing, (currently 350ms,
grepped from other places in the office).
It will be shown in a running open SlideShow (and
evtl. trigger an effect at the Object as Preview).
This will allow to get a good preview for how it
looks in the SlideShow.
This is also done for further EditViews opened
for that Document. This was not done before. It
is fine-tuned to do this only for the Views besides
the EditView with the running TextEdit to not
cause slowdowns in that active view - the TextEdit
is already running on the Overlay to have no
problems with speed, this needs to be preserved.
I had to fix a multi-view error in the a11y stack
that implied that only one view exists and thus
has to have an Outliner - that is wrong for
multiple views, only one will have one.
Change-Id: I781d32a8fcb8732ee8fcfbae72c02d1f99b6cd06
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/164160
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
The current version suppressed text for Objects
in edit mode for the paint in the view since that
text is painted/visualized by the EditEngine/Overlay
while it is being edited, so that would be displayed
twice - and in different states.
That is correct, but e.g. also suppressed display
for e.g. an opened second Window for the Document or
in a running SlideShow.
The mechanism uses embedding the Text in case it gets
edited to a simple Primitive that then can be
suppressed by the view rendering it. It decomposed
to empty, thus to visualize it a renderer had to
actively identify and process it.
I now turnedd this around - it is a normal
GroupPrimitive2D and decomposes to the text content,
so will be visualized by all renderers that do not
actively suppress it. To actively suppress it I added
a get/setTextEditActive marker to ViewInformation2D
that will be used by the ObjectContactOfPageView
to signal exactly that and to suppress in the pixel
based VCLRenderer in that case.
This is important e.g. for 2nd view window, but also
for PDF export with active TextEdit (yes, happens)
and SlideShow.
There was also support missing for an up-to-now
empty/new object (no text yet) so that text from the
active TextEdit was not provided, corrected that.
Change-Id: I0d8befdb023028d78ce341091331e9a83a0173bd
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/163773
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
There are many ways the SlideShow may be stopped,
but with IASS mode less are needed. Identified
quite some and added needed code.
Also added that for setting transitions or adding
an effect to an Object when the SlideShow is up
(the FullScreen one, makes no sense in-place)
the Preview for that is show in the running
SlideShow by switching to it, showing, and back.
So this is used in this mode as a permanent
preview for those changes.
Change-Id: I5cc34e230358aecad30fec8aee05acfa937f9270
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/163669
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
With ItemInfoPackages we now can have a buffered, static
global translation table from SlotIDs to WhichIDs since
the ItemInfoStatic used already contains all the needed
information.
Register that in registerItemInfoPackage at the Pool and
use it for lookup. That speeds up the lookup from O(n)
to O(1). Since that lookup is used in UI and UNO API
implementations this has positive effect on load/safe,
but also all interactive stuff in the whole office.
NOTE: I tried to use a merged version of that translation
table in the parent pool, but this shows double SlotIDs,
what is no wonder since that's what those are used for:
To get different WhichIDs for a SlotID in Item handling.
This *might* prevent getting rid of the chanined Pools
at all - sadly. The returned WhichID directly depends
on which Pool the function(s) GetWhichIDFromSlotID and
GetTrueWhichIDFromSlotID are called.
NOTE: Very strange is that the parameter 'bDeep' in that
functions is *not* passed down to the call to the
secondary Pool - probably an error, but risky to fix,
that may change already the behaviour :-(
Change-Id: Iea77ffad0f6a5401ab74fea0bbfc2589c66680ea
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/163597
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
The idea is a long wanted feature, to allow the
EditView to stay active during a running Presentation
so that you can do changes which then directly show
up in the running presentation. This will need quite
some steps. To be on the safe side all of the changes
are isolated by a variable to cause no harm. For
now this uses ENVVAR ENABLE_INTERACTIVE_SLIDESHOW,
but that may change e.g. as config entry or option.
Change-Id: Id0ab9a473603225456206fbd933e932248681509
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/163514
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
Driving forward the Item reworks I now take the
ItemPool in focus: It now does not hold any items
anymore and should be renamed to something like
ItemInfoProvider/ItemHelper, since it's main purpose
is to provide the Defaults for the Item functionality.
There is that SfxItemInfo, only a struct and bundling
SlotID and ItemFlags. There are also the DefaultItems,
just handled as ptrs in an array. It is/was always
error-prone to keep these in sync. Remember that it's
also necessary for the order to not only being sorted
but being increments of one with no gaps allowed in
the WhichIDs to which the Items are bound.
I now bundled that to a new class ItemInfo that joins
WhichID, SlotID, ItemFlags and the default Item. This
is a pure virtual base class, it comes in three
derivations:
(1) ItemInfoStatic:
This is supposed to be global static and hosts the
Item in a std::unique_ptr to ensure cleanup. It is
designed to be constructed once during runtime and
being shared globally. It allows the ItemPtr to be
nullptr to mark as non-static (if initial static is
not possible for some reason) but still offers the
needed data. Most cases (95%+) are of that case.
The contained Item is owned by that instance. The
flag isStaticDefault() is set at the Item.
(2) ItemInfoDynamic:
This is supposed to be used for cases where the Item
cannot be static: Mainly for SfxSetItem (that needs
a Pool itself in the contained SfxItemSet, so lifetime
is bound to that Pool), but other cases showed up in
the transition. These instances live while the Pool
lives and get destructed when the Pool goes down.
Also uses std::unique_ptr for the Item instance for
as much automated cleanup as possible, the contained
Item is owned by that instance, the instance by the
Pool. The flag isDynamicDefault() is set at the Item.
(3) ItemInfoUser:
This is used for UserDefaults that can be set for
every ItemInfo entry to 'overshadow' the default
from the 'outside'. It uses a regular Item and
the central access methods implCreateItemEntry/
implCleanupItemEntry to manage the Item instance,
thus works like a SfxPoolItemHolder. The Item
instance can be globally shared and re-used even
when the Pool goes down. Instances belong to the
Pool and are cleaned up when the Pool goes down.
This Item does not need any further flag to be
set.
The ItemInfos are organized using a class
called ItemInfoPackage:
This bundles groups of ItemInfoStatic to
functional instances. There are derivations/
implementations of this e.g. for Writer ItemPool
bundling all the needed defaults for Writer,
similar for draw/impress, Calc and other usages.
These ItemInfoPackage can be 'registered' at an
ItemPool using it's method registerItemInfoPackage.
This does all the needed stuff to setup that
group of ItemInfos at the Pool (It even sets
internal vars First/LastWhich, that info can just
be derived from the buildup ItemInfo Ptrs).
The ItemInfoPackage has methods 'size()' and
'getItemInfo(index) to allow looping over it
and deliver the infos the Pool needs. The
(forced, pure virtual) overloads of getItemInfo
in the specific implementations check for the
ItemPtr being nullptr and create a exclusive
incarnation of ItemInfoDynamic for the Pool if
needed, returning that. The Pool owns the
ItemInfoDynamic incarnations and uses the
ItemInfoStatic directly. On shutdown it cleans
up the ItemInfoDynamic as needed.
The ItemInfoUser is used by the Pool when a
UserDefault is set/used: for SetUserDefaultItem,
GetUserDefaultItem, ResetUserDefaultItem. It
is not held in a 2nd list, but directly in the
list of ItemInfo'ptrs: To keep track of this
an unordered_map is used that helds the original
ItemInfo associated with the WhichID. That way
no two lookups (as before) are needed to get the
current Pool's default for any WhichID.
The derivations of ItemInfoPackage are
encapsulated and just allow access to an
ItemInfoPackage& with a single method as
return value. All use a static local instance
of a std::array<ItemInfoStatic, FIXED_SIZE>
which constructs all ItemInfoStatic and the
static Item instances - if already possible.
Sometimes it is necessary to overload the
constructor to set some static instances
for Items later than the lib init. These are
also just marked with nullptr as Item instance.
Some need to overload getItemInfo to complete
instances of ItemInfoStatic, if needed, or
create and deliver instances of ItemInfoDynamic.
The registerItemInfoPackage also offers a
optional lambda callback: there were two cases
where local data from the Pool was needed to
incarnate the item - just add that to the
call to registerItemInfoPackage if needed,
see examples in the adapted code.
For the re-use of Items this means that now
in SfxItemSet/SfxPoolItemHolder *true* static
Items can and will be used without RefCount
directly and globally. This is also the case
for dynamic Items, with the exception of
differing Pools for SfxSetItems which cannot
be done.
Future:
That design is already prepared to allow
solving that Pool-chaining problem: currently
there are master/sub-pools and all accesses
have to traverse that structure before even
doing anything.
For the future the idea is more to 'compose'
a Pool by registering ItemInfoPackages, e.g.
for Writer pool you may start with SfxItemPool,
register the writer-specific ItemInfoPackage,
then the one for DrawingLayer (if needed) and
the one for EditEngine.
It should also be possible to get to smaller
granularities of that packages. Ideas for
new ones will emerge. We might also think
about composing Pools which can e.g. run Writer
and Chart, so allowing to use Chart *without*
OLE stuff in Writer - just ideas...
More changes:
- Adapted all stuff, cleaned up old stuff/
definitions
- Removed FreezeIdRanges, that can be done
once per Pool on-demand (and cannot be
forgotten to be called)
- Merged XOutdevItemPool with SdrItemPool
and offered a ItemInfoPackage which joins
both needed sets of Items
- All the cleanup hassle with Pools and
defaults cleaned up
- Adapted all access methods of the pool
to use that new stuff. Pool chaining
currently stays, but I use a central
method 'getTargetPool' instead of
recursive calling to get the correct
Pool for the action
Change-Id: I2b8d3d4c3cc80b1d0d0b3c0f4bd90d7656b4bab7
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/163157
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
Sorted out some methods at ItemPool which process
Defaults to make more clear what is going on and
what which method is doing.
Change-Id: I2568d3e03d0a56a14b6fe4e04521e1a8e22c000b
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/162643
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
Unfortunately I had overseen something with derived
classes, but it came now up on CI ASan/UBsan build
with a failing UnitTest - thanks to pointing me at
it.
The ItemInstanceManager at the SfxPoolItems are now
no longer static but constructed instances returned
on-demand.
Also added checks to really use an incarnated/
registered one *only* for that derivation and made
sure this is now correctly supported.
Had to change again, using createItemInstanceManager
to always create instances was less effective than
intended, back to getItemInstanceManager & static
instances in the Item implementations. Also added
some stuff to implCreateItemEntry/implCleanupItemEntry
to be more effective, e.g. direct handling of
slot stuff in latter one. Also some more asserts
and comments. Slot stuff is now handled without
RefCounting, takes some write accesses away...
Change-Id: I6cd69556b416510b5b23549dd042ff3ba155559d
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/162521
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
As addition to tdf#158605 I have added a
global default mechanism to support global
sharing of Item instances. It's automated
and complements the existing one, see one
of the last commits. All in all there are
now the following possibilities to support
this for individual Item derivations:
(1) Do nothing: In that case, if the Item
is shareable, the new mechanism will kick
in: It will start sharing the Item globally,
but not immediately: After a defined amount
of allowed non-shared ocurrences (look for
NUMBER_OF_UNSHARED_INSTANCES) an instance
of the default ItemInstanceManager, a
DefaultItemInstanceManager, will be incarnated
and used. NOTE: Mixing shared/unshared
instances is not a problem (we might even
implement a kind of 're-hash' when this
kicks in, but is not really needded).
(2) Overload getItemInstanceManager for
SfxPoolItem in a class derived from
SfxPoolItem and...
(2a) Return a static incarnation of
DefaultItemInstanceManager to immediately
start global sharing of that Item derivation.
(2b) Implement and return your own
implementation and static incarnation of
ItemInstanceManager to do something better/
faster that the default implementation can#
do. Example: SvxFontItem, uses hashing.
The NUMBER_OF_UNSHARED_INSTANCES is currently
at (50) which gives a decent relationship
bewteen no global sharing (speed) and memory
needs. Not sharing is faster (that access to
'find' an equal item is spared which might be
costly), sharing again needs less memory.
There are two supported ENVVARs to use:
(a) SVL_DISABLE_ITEM_INSTANCE_MANAGER:
This disables the mechanism of global Item
sharing completely. This can be used to test/
check speed/memory needs compared with using
it, but also may come in handy to check if
evtl. errors/resgrressions have to do with
it.
(b) SVL_SHARE_ITEMS_GLOBALLY_INSTANTLY:
This internally forces the
NUMBER_OF_UNSHARED_INSTANCES to be ignored
and start sharing ALL Item derivations
instantly.
Change-Id: I40d9c5f228f0bcbf239f2ce0a02d423054240570
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/162478
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
The task shows that the commit including the Item
paradigm change has follow-ups: It now does no
longer try to share Items as much as possible
(detailed reasons in that commit). Mainly for speed
reasons since that sharing was done before by
(mostly) linearly searching in existing instances
registered at one Pool, using the operator== of the
SfxPoolItems. That costs runtime.
There is somewhere a sweet-spot between memory and
runtime: the number of Items allocated and the time
spent to share them more effectively. This task shows
- despite being a non-real-world document - that
for extremes like this putting work in sharing is
still needed.
But there are possibilities to combine both: If we
can implement solutions that do not need much time
to ideintify an aleady existing instance we will get
the best of both worlds.
As explained already in that change, if we would
need that again, then on a better base. Thus I drove
forward ITEM changes to a state where we are now able
to share Items globally in the office - not per pool
but for all ItemSets/ItemHolders and thus all Apps/
Models/opened documents.
NOTE: This currently needs to include the WhichID
that is included in the Item, so cannot share pure
Item-data (as the old usage did too). This does not
need to stay that way: If you think about it, the
association between WhichID and Pool/Holder is
defined in Pool/Holder, so theoretically the Item
does not need to contain the WhichID. This will
be hard to do due too many places in the code that
use the WhichID stored at the Item.
To support that I added an ItemInstanceManager with
a simple interface (find/add/remove) and it's usage
in the two central Item-existance managing methods
implCreateItemEntry/implCleanupItemEntry. The
interface is pure virtual and all methods private,
only the mentioned managing methods are allowed to
access these. Also added a virtual method to
SfxPoolItem called getItemInstanceManager() that
can be implemented by Items that want to support
that.
Also added a default implementation of
ItemInstanceManager called DefaultItemInstanceManager
that uses linear search using operator== from the
Item that can be used/added to every Item easily.
It works for all Items and does in principle what
the former implementation does. It is intended as
simple/fast fallback.
I also added a statistic element to measure the most
used non-RefCounted Items on an Office-run, this
will be printed at office shutdown using SAL_LOG
and the 'svl.items' flag.
I then checked all Items that were used in this
error/bug scenario that used an extensive number
of incarnations and added an ItemInstanceManager
for these.
For SvxFontItem I added one that creates a hash and
thus needs not to search for instances at all, with
the caveat that the WhichID needs to be included.
Thus the hash is not at the Item, but only in the
ItemInstanceManager implementation.
For SfxBoolItem I implemented one that hashes using
the WhichID and holding both possible states in an
associated std::pair, true and false, thus the
SfxBoolItem is identified fast and only two instances
per WhichID exist (when used in Pool/Holder).
For 11 other Items I just added using the standard
implementation, DefaultItemInstanceManager. Of
course the more we optimize the better it will get.
For all Items where I added that mechanism I also
added ASSERT_CHANGE_REFCOUNTED_ITEM to all write
calls/methods for that Item. It asserts when the
RefCounted Item is to be changed. This should be
done in all write accesses to Items, but we have
ca. 500 derivations.
NOTE: There was *one* place I found where that
was already done -> was alredy seen as problem,
but not really adressed.
Despite this example file is not representative,
it is still a start to init this new instance
re-use of Items.
I am already thinking about doing that globally,
depending on the usage number (maybe combined with
sizeof(item)). There is no argument to not even
change strategy at runtime when a specific number
of incarnations of one Item derivation is reached,
but this is not part of this fix.
Change-Id: Ie56cf40e0341f98dcbf29cf5c06368316829415e
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/162402
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
An instance of SfxVoidItem(0) was used to signal
the SfxItemState::DISABLED. This was done not only
using WhichID == 0, but using isVoidItem() at the
SfxPoolItem. Unfortunately this mixes up with usages
of SfxVoidItems, mostly for UI stuff/Slots.
This also means that all the time an SfxVoidItem
had to be cloned/delete when when added/removed
from ItemSet or ItemHolder. Much more action than
e.g. for INVALID_POOL_ITEM which we already use
by havong just a simple ptr to a single static
instance of an Item. Disabled should do the same
thing.
Unfortunately also the functionality was mixed
with non-SfxItemState::DISABLED purposes and these
were very hard to be separated.
But the current solution works now after some
quirks doing that. It even oes no more need the
isVoidItem() flag at the SfxPoolItem.
Change-Id: I99f03db144f541ae4ea35f3775b3b3d58a375a81
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/162414
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
After some experiments I now can remove
DirectPutItemInPool and DirectRemoveItemFromPool.
With the changes done before it is now possible to
get rid of this 'compromize'. Now there are no
more Items held at the 'Pool' which now can be
developed in the direction of ssth like
'SfxItemSupport' - what it really is.
Some of the last usages of DirectPutItemInPool were
in SvxAreaTabDialog::SavePalettes(), so I tried to
trigger those cases with using LO and calling that
Dialog in all situations I could possibly think
about, but it was never used.
Then I added asserts and run a UnitTests in the apps,
also no luck. Thus I would guess these are not used.
These put changed stuff from the Dialog 'directly' to
the Pool (what is not really supported and is a hint
for a 'compromize' that some functionality did not
find/want to use the right spot in the model to save
and hold that data). Thus it *would* be accessible
using the SurrogateMechanism at the Pool (which is
also a 'compromize', see some of my other commits),
but I also found no hints at places where that is
done.
Thus I decided to create this change and let's see
if that asserts ever get triggered in the builds
or tests.
Indeed did not trigger. I checked what other places
do which use SfxObjectShell::Current() when they get
no DocShell: Most avoid doing something, but none
puts stuff to the Pool, so I go one step further
and do what other places do: warn and return. Also
simplified SvxAreaTabDialog::SavePalettes() as then
feasible.
Change-Id: I1c351eac4ada26288a56a69d57008a09f2d19121
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/162340
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
It is used in SC, DirectPut* in Pool, fetched using
Surrogates, all bad. Only to transport data over the
Pool, may not even need to be an Item.
Trying to solve/losen that gordian knot, looks good.
Is now a normal data holder class, could find a good
parent for it that the Dialog and the instances
setting up/using that Dialog can use.
Forgot to reset that data instance in one place, but
also checked in-between a version that still used the
Item to excluse that the Poolis the same, but the
ScTabViewShell does change. FOund an error with
SfxPoolItemHolder when reseting, also changed.
Change-Id: I1c99d675d1cc3d21205c3e2df78d4b52a696e7ee
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/162313
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
ItemSurrogates are a possibility to iterate over
all SfxPoolItems associated with a WhichID at a
ItemPool to collect or change data.
It is in general not a good idea: the correct action
would be to iterate over the model data and change/
adapt/collect data on the Items of the type
in question. This is because the *assumtion* that you
iteate over all the Items associated with a document
model is *not* correct:
The ItemPool of the model is used for various ItemSets,
e.g. Dialogs/Sidebar and others, so you might get Items
not only from the DocumentModel but from elsewhere.
You might even get Items from *other* models, so
changing these might have unpredictable effects (!)
It is clear to me that this mechanism is more convenient
that iterating over the document models, and it might
have been invented due to this and then used in more
and maore cases. There should be a lambda/callback-based
mechanism in every document model to do this. Until then
we have to live with this 'compromize'. There are over
100 usages currrently, so no way to easily replace this.
For those reasons I changed this to be more safe: There
are two methods to do this now:
1: iterateItemSurrogates to allow read/write access. I
identified six places where that mechanism was used
to change SfxPoolItems, with the use of const_cast.
This now offers a lambda/callback mechanism and the
needed data in a helper (SurrogateData). Internally
it iterates over ItemSets and ItmHolders registered
and thus associated with the Pool. Changing an Item
means to set a changed Item at the Pool/replace the
holder.
2: GetItemSurrogates/FindItemSurrogate to allow
read-only iteration (2nd one with filter). This
collects the Items in a vector that you provide over
which you can then iterate. Do *not* use const_cast
and change the Item when using this (!)
Note that mechanism (2) pe-filters the Items so that
you get each only once: Of couse one Item can be set
in more than one ItemSet/Holder (but also in more than
one model). This filtering is not possible for (1), you
have to evtl. do the same replace action for the same
item, but this is the only way to not change Items that
are associated wth another model.
Note that (2) could also be changed to a lambda/callback
mechanism similar to (1), but there are too many places
that would beed to be adapted. That would have the
advantage to not need to pre-collect the candidates in a
first run.
Also removed/replaced FindItemSurrogate with using
GetItemSurrogates and locally filtering with that needle.
That also made me remove/cleanup CollectSurrogates, it's
only used in one place now.
Change-Id: I0fe2f51f4fca45e1e5aea032cb96bb77b4567f4d
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/162254
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
The ::Put methods at SfxItemSet had an extra WhichID
parameter that was not really documented, but I would
guess often asked why it exists: An extra WhichID, just
called 'nWhich' (which makes things NOT clearer). That
is 'strange' since the Item given to be put already
internally has a WhichID, so why a 2nd one?
If you were really interested and read all that code
(no, no comments on that anywhere) you might know
that this a kind of 'Target-WhichID' under which
the Item shall be put to the ItemSet. Since this
is unclear for most people it is even dangerous and
explains why so many code places just hand over the
WhichID requsted from the Item that already gets
handed over.
To make it short: I removed that. For the 19 places
where this was really needed I added a new method
besides ::Put called ::PutAsTargetWhich that takes that
extra WhichID (now called TargetWhich) and takes the
needed actions. These are quite some because that may
be combined with the bPassingOwnership flag, see new
SfxItemSet::PutImplAsTargetWhich method.
This makes usage of ItemSets/Items less dangerous. It
also simplifies and thus makes safer the central helpers
implCreateItemEntry/implCleanupItemEntry which have some
less cases to handle.
Debugged the failing UnitTests showed that there is
an incarnate Item != SfxVoidItem that causes problems.
I checked for errors in the change, but no luck.
Afterr some time I found out that a ::Clone
implementation caused the problem: These need to
also copy the WichID of the original, but the
SfxFrameItem failed to do so. This did not cause
problems in the former version because
implCreateItemEntry was designed to set a missing/
different WhichID.
I corrected that in SfxFrameItem, also removed not
needed costructor that caused that. Also added a
SAL_WARN and a correction in implCreateItemEntry.
I could have added an assert (did so for running
local UnitTests), but should be enough.
NOTE: When hunting for Items except SfxVoidItem
that get crerated using a WhichID '0' i learned
that this indeed happens: There are some (5) calls
to SfxRequest::SetReturnValue that incarnate an
SfxBoolItem with WhichID '0' (ZERO). This is not
good and I think about how to change that...
Change-Id: I9854a14cdc42d1cc19c7b9df65ce74147d680825
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/162124
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
Currently a Pool-Attribute (in SfxItemInfo), but should
be a SfxPoolItem property. Originally 'moved' from old
'poolable' used as hint. I identified needs more general
and moved it to where it belongs.
Also reworked SfxItemInfo to no longer have/support
single bollean flags, but a FlagVariable and defined
SFX_ITEMINFOFLAG_* entries to access these, that will
make future changes easier without having to change
all palces where these get defined over and over again.
Added CheckItemInfoFlag for gereral access to that flag
and e.g. NeedsSurrogateSupport to directly check for
the SFX_ITEMINFOFLAG_SUPPORT_SURROGATE flag as syntactical
sugar, that makes the intention clear.
Change-Id: I09c238c7c5b7f721b657d7b0a44dbc8d14e02528
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/161982
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
That item was never 'pooled', so operator== was not really
ever used. It just compared the 'type', so pretty many
instances were assumed to be equal, what is wrong.
We discussed to implement it (there is quite some content),
but we came to the point that it's only safe to say
instances are equal when same instance -> fallback to ptr
compare.
This came into play since I identified/changed many (160?)
places where SfxPoolItems were ptr-compared when doing that
paradigm change in Items. This leads to the two methods
'areSfxPoolItemPtrsEqual' which just makes ptr compare and
'SfxPoolItem::areSame' which also will use op==. For the
initial adaption I chose the wrong function adapting
places where SwTOXMark were involved.
Change-Id: I7df029ad4542719681b1455de17ed5990d248395
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/161963
Reviewed-by: Michael Stahl <michael.stahl@allotropia.de>
Tested-by: Armin Le Grand <Armin.Le.Grand@me.com>
The issue is that the flag RegisteredAtPool
at the SfxPoolItem is Pool-dependent: It marks that
the Item is registeres at *one* Pool/Model. This
makes it Pool-dependent. Due to this there is no way
to share Items that need to be registered globally/
in multiple Pools/Models what is one of the goals
for optimal sharing.
We can also not live without having access to all
Items associated with the Pool, due to mechanisms
in place like the Surrogate stuff.
This again is used for two purposes:
(1) Access all Items associated with one Pool/Model,
at least that is the assumption. This is not valid
since it gets corrupted with a single ItemSet/Holder
used that does not host model data, e.g. an open
Dialog or the Sidebar (or...). But works in
principle.
(2) Access extra-Items that are held nowhere and
are created using DirectPutItemInPool, e.g. infos
for a Dialog. These would need a instance/place to
host them, the Pool is (ab)used for that.
Both are 'compromizes' (to not use a more bad word)
and should not exist. (1) should iterate over the
Model and do actions. There are even places that
use (1) to *change* Items, by casting them to
non-const, even RefCounted ones, so having no control
over what all might be changed doing so. Since we
talk about ca. 100+ places there is no way to get
away from this - I can imagine how this happened:
The 'poolable' attr traditionally needed for the old
binary format was one day 'used' to not need to
iterate over the Model, an API was added to access
and this usage was copied. Sigh..
It is even used when ODF is loaded: E.g. the
FillStyle is imported from XML, interpreted, and
put into an ItemSet. Then it gets set at the
XShape using UNO API and a *name* -> that name and
the Surrogate mechanism is used to find and set the
FillStyle at the Model Object. The FillStyle could
probably just be set using UNO API and the data
directly.
The association between Model/Pool and Item is
created by the object hosting the Item, these are
ItemSets and ItemHolders. Thus it is possible to
register these at the Pool. This allows to iterate
and collect the Items associated with the Pool
and keep the Surrogate mechanism alive.
This is the main change done here. It limits
the registrations to Items for which (at the Pool)
the NeedsPoolRegistration is set, also
Item-independent. Speed is okay, I saw no big
changes in my tests here. The registration is
just pointers, no ownership or RefCounting needed
here.
The advantage is that Items get closer to be
shared office-wide, they can be referenced by
multiple ItemSets (RefCnt) associated with
different Pools/Models.
NOTE: This is not true for SfxSetItems, these are
and will stay Pool-dependent due to their need
to a Pool in the contained ItemSet.
Note that we have ca. six deivations of SfxSetItem,
but ca. 500+ Item derivations, so not too bad.
For the usages of Surrogates to change existing,
RefCounted Items: These can now at least be
changed - if they show up to be problematic - to
iterate over the registered ItemSets and change
Items there the correct way: Set a changed one
at the ItemSet. That also allows Objects to
*react* on ItemChanges, there is no way to do
that with the existing 'compromize'...
UnitTests show that this already works well for
SC and SD, but SW has still some issues. I will
put this to gerrit now, but there will be
additional work.
A involved problem is the current DefaultItem
handling and the state the Pool implementation
is in. E.g. StaticDefaults are not really static,
Pools hard-delete the DefaultItems (forcing the
RefCnt to zero to not have the destructor
complain) and other quirks. Looking at that
right now, hoping to get this change done without
having to change that too much.
I thought about adapting PoolItemTest to this,
but it is only related to DirectPutItemInPool
which is mostly gone and hopefully completely
soon.
Nonetheless I adapted that mechanism to use a
list of SfxPoolItemHolder at the Pool. That makes
it safe and abandons the need for indirect
garbage collection removal at the Pool.
Change-Id: Ib47f21dafa989202930919eace5f7e9c5632ce96
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/161896
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
The impl SlideshowImpl gets notifications for changes from
the ObjectModel, but needs to ignore them when SlideShow
is not active/running. For discussion how to do this see
comments in task.
Change-Id: Ic251af4b82f0f4b48d8d9b0127dd2f6015966935
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/161922
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
SwTextAttr uses a 'const SfxPoolItem*' and the
mentioned DirectPutItemInPool/DirectRemoveItemFromPool
to force creation of a ref-counted SfxPoolItem to
work with. There is now tooling to do so, so changed
to using SfxPoolItemHolder. That works like a small
SfxItemSet - it transforms the Item to ref-count
form and takes over management. It is safe to
pass around, cheap to copy and it does not need manual
cleanup of SfxPoolItems (automatic -as in SfxItemPool).
Change-Id: I41401d5144c9cd59d5b10362ea17d63e35ab1eb3
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/161799
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
Now trying not to delete a LocalCallbackTimer if it gets
used - no limitation since this only happens when a
time is given (maCallbackSeconds).
Also added to reset callback ptr (clearCallback()) to
nullptr and make onShot dependent of that, hope is that
destruction of LocalCallbackTimer in
~BufferedDecompositionPrimitive2D gets safer that way,
excluding race conditions.
Change-Id: Id474962bdae14631afc42d2d92db635408fb67d9
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/161434
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
Seems to be an error in the preparation to Bitmap conversion
in VclMetafileProcessor2D::processTransparencePrimitive2D.
While for calculating the LogicRange maCurrentTransformation
is used (and that is correct) it is *not* used in the embedding
for calling convertToBitmapEx.
It is correct to get back to logic/world coordinates for
calculating the embedding, but maCurrentTransformation is
needed to define the correct full embedding.
I might have overlooked that in the original change because
maCurrentTransformation is local to the VclMetafileProcessor2D
in action right at that moment and is usually empty for simple
cases, but - as can be seen - not always.
Change-Id: Ib2deaa2c8328655ecc80964ba2e23e67964ac3ef
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/161409
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
ScPatternAttr is traditionally derived from SfxPoolItem
(or better: SfxSetItem) and held in the ScDocumentPool
as Item.
This is only because of 'using' the 'poolable'
functionality of the Item/ItemSet/ItemPool mechanism.
Lots of hacks were added to sc and Item/ItemSet/
ItemPool to make that 'work' which shows already that
this relationship is not optimal.
It uses DirectPutItemInPool/DirectRemoveItemFromPool
to do so, also with massive overhead to do that (and
with not much success). The RefCnt in the SfxPoolItem
that is used for this never worked reliably, so the
SfxItemPool was (ab)used as garbage collector (all
Items added and never removed get deleted at last
for good when the Pool goes down). For this reasons
and to be able to further get ItemSets modernized
I changed this. I did two big changes here:
(1) No longer derive ScPatternAttr from SfxItemSet/
SfxSetItem, no longer hold as SfxPoolItem
(2) Add tooling to reliably control the lifetime of
ScPatternAttr instances and ther uniqueness/
reusage for memory reasons
It is now a regular non-derived class. The SfxItemSet
formally derived from SfxSetItem is now a member. The
RefCnt is now also a member (so independent from
size/data type of SfxPoolItem). All in all it's pretty
much the same size as before.
To support handling it I created a CellAttributeHelper
that is at/owned by ScDocument and takes over tooling
to handle the ScPatternAttr. It supports to guarantee
the uniqueness of incarnated ScPatternAttr instances for
a ScDocument by providing helpers like registerAndCheck
and doUnregister. It hosts the default CellAttribute/
ScPatternAttr. That default handling was anyways not
using the standard default-handling of Items/Pools.
I adapted whole SC to use that mainly by replacing calls
to DirectPutItemInPool with registerAndCheck and
DirectRemoveItemFromPool with doUnregister, BUT: This
was not sufficient, the RefCnt kept to be broken.
For that reason I decided to also do (2) in this change:
I added a CellAttributeHolder that owns/regulates the
lifetime of a single ScPatternAttr. Originally it also
contained the CellAttributeHolder, but after some
thoughts I decided that this is not needed - if there
is no ScPatternAttr set, no CellAttributeHolder is
needed for safe cleanup at destruction of the helper.
So I moved/added the CellAttributeHolder to ScPatternAttr
where it belongs more naturally anyways. The big plus is
that CellAttributeHolder is just one ptr, so not bigger
than having a simple ScPatternAttr*. That way, e.g.
ScAttrEntry in ScAttrArray did not 'grow' at all. In
principle all places where a ScPatternAttr* is used can
now be replaced by using a CellAttributeHolder, except
for construction. It is capable to be initialized with
either ScPatternAttr instances from the heap (it creates
a copy that then gets RefCounted) or allocated (it
supports ownership change at construction time).
Note that ScAttrEntry started to get more a C++ class
in that change, it has a constructor. I did not change
the SCROW member, but that should also be done.
Also made registerAndCheck/doUnregister private in
CellAttributeHelper and exclusively used by
CellAttributeHolder. That way the RefCnt works, and a
lot of code gets much simpler (check ScItemPoolCache,
it's now straightforward) and safer and ~ScPatternAttr()
uses now a hard
assert(!isRegistered());
which shows that RefCnt works now (the 1st time?).
There can be done more (see ToDo section below) but I
myself will concentrate on getting ItemSets forward.
This decoupling makes both involved mechanisms more safe,
less complex and more stable. It also opens up
possibilities to further optimize ScPatternAttr in SC
without further hacking Item/ItemSet/ItemPool stuff.
NOTE: ScPatternAttr *should* be renamed to 'CellAttribute'
which describes what it is. The experiencd devs may know
what it does, but it is a hindrance for understanding for
attacting new devs. I already used now names like
CellAttributeHelper/CellAttributeHolder etc., but
abstained from renaming ScPatternAttr, see ToDo list below.
SfxItemSet discussion:
ScPatternAttr still contains a SfxItemSet, or better, a
SfxSetItem. For that reason it still depends on access to
an SfxItemPool (so there is acces in CellAttributeHelper).
This is in principle not needed - no Item (in the range
[ATTR_PATTERN_START .. ATTR_PATTERN_END]) needs that.
In principle ScPatternAttr could now do it's own handling
of those needed Items, however this might be done (linear
array, hash-by-WhichID, ...). The Items get translated
to and from this to the rest of the office anyways.
Note that *merging* of SfxItemSets is *still* needed what
means to have WhichID slots in SfxItemState::DONTCARE,
see note in ScPatternAttr::ScPatternAttr about that. And
there is also the Surrogates stuff which would have to be
checked.
The other extreme is to use SfxItemSet *more*, e.g. directly
derive from SfxItemSet what would make stuff easier, maybe
even get back to using the 'regular' Items like all office,
I doubt that that would be much slower, so why...?
Also possible is to remove that range of Items exclusively
used by ScPatternAttr from ScDocumentPool *completely* and
create an own Pool for them, owned by CellAttributeHelper.
That Pool might even be static global, so all SC Docs could
share all those Items - maybe even the ScPatternAttr
themselves (except the default per document). That would
remove the dependency of ScPatternAttr from a Pool
completely.
ToDo-List:
- rename ScPatternAttr to CellAttribute or similar
- use SfxItemSetFixed with range [ATTR_PATTERN_START
.. ATTR_PATTERN_END] instead of regular SfxItemSet
(if the copy-construtor works now...?)
- maybe create own/separate Pool for exclusive Items
- make ScAttrEntry more a C++ class by moving SCROW
to the private section, add get/set methods and
adapt SC
Had to add some more usages of CellAttributeHolder
to the SC Sort mechanism, there were situations where
the sorted ScPatternAttr were replaced in the Table,
but the 'sorted' ones were just ScPatternAttr*, thus
deleting the valid ones in the Table already. Using
CellAttributeHolder makes this safe, too.
Added a small, one-entry cache to CellAttributeHelper
to buffer the last found buffered ScPattrnAttr. It
has a HitRate of ca. 5-6% and brings the UnitTest
testSheetCellRangeProperties from 0m48,710s to
0m37,556s. Not too massive, but erery bit counts :-)
Also shows that after that change optimizations in
the now split functionality is possible and easy.
Change-Id: I268a7b2a943ce5ddfe3c75b5e648c0f6b0cedb85
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/161244
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
...and adapted code to make use of it. This makes checking if
the SfxPoolItemHolder instance contains an Item or not simpler
and thus more readable, no need to access the Item* every time
using getItem() - is okay, but not intuitive.
Change-Id: I8042267cce670aca2641a91cd36285058f17ffbb
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/161380
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
There seem to be problems with re-creation of
SdrCustomShapePrimitive2D primitive presentation
due to SdrText not being held as
uno::Reference<uno::XWeak> everywhere. For now,
try if excluding SdrCustomShapePrimitive2D from
being flushed helps - may give hints already...
Change-Id: I58373f7dcd81dd24fbf7a0687bb782326ad290fd
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/161190
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
There are some CrashReports in 7.6 which have
DeleteItemOnIdle on the stack, but there is nothing
reproducable. So I took a look...
I first thought it's a MCGR regression, due to classes
on the stack. But the Item involved is just random, can
happen with any Item.
Then I thought it may have to do with ITEM refactorings,
but it happens with DeleteItemOnIdle involved, so also
not the case. I already saw DeleteItemOnIdle when doing
these and qualified as 'hack' in the way. already
It is only on Windows and DeleteItemOnIdle is involved.
This again (took a deeper look now) is an old hack to
keep an SfxPoolItem 'alive' for some 'time'. For that,
it triggers an async reschedule which then deletes the
Item when being called. If the Item will be used after
that is pure coincidence - seems to work in most cases.
It seems as if for Windows the timing slightly changed
for some scenarios, so a reschedule is too early. This
can happen with this hack anytime.
DeleteItemOnIdle is used in scenarios where SfxPoolItem*
is e.g. returned, but is *not* anchored, so e.g. not
member of an SfxItemSet. Or in short: Lifetime is not
safe.
DeleteItemOnIdle exists since 1st import, but was
changed to AsyncEvent ca. 4 months ago (see
57145acf9e), so that may
have caused it. It is possible that these errors happen
on Windows since then. Before something more complicated
was used to delete it late, but surely also not really
safe.
Due to ITEM refactor I have the knowledge/tooling to
solve this. It will not be a 1-5 lines fix, but it is
a hack and in the way for further ITEM refactor anyways.
What we have nowadays is a SfxPoolItemHolder -> it's
like an SfxItemSet for a single Item. It safely holds/
controls the lifetime of an SfxPoolItem. It is already
used in quite some places. It helps to solve many hacks,
also the ones putting Items directly to the Pool - due
to there never was an alternative for that. In principle
the ItemPool/ItemSet/Item paradigm was never complete
without SfxPoolItemHolder.
Thus I started to fix that (and remove that hack for
good, sooo many changes over the years, sigh), but as
said is not straightforward. Will have to change
retvals of involved stuff to SfxPoolItemHolder - it's
just two pointers and designed to be copied (one is a
Pool, needed to cleanup when destructing).
CopyConstruct/destroy just counts the RefCnt up/down,
so cheap.
1st version compiling, let's check on gerrit...
Corrected one error in QueryState for securitypage, also
added some security features/asserts.
Change-Id: Ida49fd35ca88ead84b11d93e18b978cb9e395090
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/161083
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
As sberg mentioned in previous change this might be responsible
for heap-use-after-free problems. I took a look and it seems
to be possible that the timer calls back while deletion of the
helper is in progress, so I try now to first stop the timer
and then delete so that it cannot trigger. Will see if that
works - if not I might have to use another lock...
Change-Id: I1ae27d9ed890f352904cab18c3292b449659a3ce
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/161128
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
There are the classes BufferedDecompositionPrimitive2D
and BufferedDecompositionGroupPrimitive2D that offer
a unified mechanism to preserve/buffer the decomposition
of a Primitive, independent of the complexity. This may
include e.g. the pixel representation of a 3D Scene,
for Charts/CustomShapes and others.
Until now these were refreshed when the objects change,
or when the buffered decomposition decides that it is
necessary, but there was no general mechanism in place
to clean these up while the Primitive owning them was
alive/used. This is not a big thing in Draw/Impress
since this would only be relevant when zooming deep into
a SdrPage, so the non-vislible parts would still hold
data that would not be needed. But for Calc and Writer
this is more relevant: A 3D Chart on the 1st page of a
Writer document would be held buffered all the time a
user would work on pages below that. This is good for
guaranteeing fast re-visualization when scrolling up
again, but until then would just block memory.
So I added this general mechanism that allows activating
flushing timer-based, on Seconds. The default is null
seconds, thus deactivated. It should only be used for
Primitives that can get expensive, not for all.
NOTE: I checked activating for all Primitives to be on
the safe side, works.
It is now (initially) activated for:
- GlowPrimitive2D
- GraphicPrimitive2D
- MetafilePrimitive2D
- ScenePrimitive2D
- ShadowPrimitive2D (softShadow)
- SoftEdgePrimitive2D
- SdrCustomShapePrimitive2D
- SdrGrafPrimitive2D
These are the usual suspects that may use some memory
to buffer their decomposition (for good reasons, each
repaint uses it). Other Primitives which may show need
to be treated this way can easily be added, just by
calling setCallbackSeconds(s) in their constructor. This
is true for all Primitives derived from the two classes
mentioned above.
NOTE: Too short buffering times are *not* useful - e.g.
for a expensive-to-be-recreated 3D chart representation
this may not pay out, so I chose times in a way that try
to respect this.
NOTE: This change needs
7397fa7cdf to work
correctly (thanks to sberg for this). Without this the
office hangs/does not terminate regularly in trying to
destroy the 'TimerManager'.
Change-Id: Id4802afcb6d12480bb2935cc1ef67fe443b3b788
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/160926
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
For this purpose allow reduced 3D quality in some circumstances
and make a compromize between quality and speed. This is
balanced between those two targets, fine-tuning/experimenting
can be done with some static local values if needed.
Change-Id: Ib00b6e9c3c3ff165d82ff12d23bf15196f0a0ee0
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/160467
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
The implementation was not aware of the used functionality
in the used call to SdrPageView::DrawLayer: The given
OutputDevice is *not* intended to hand over a new render
target, it's used to allow the mechanism(s) to *identify*
one of multiple possibe registered render targets (which are
usually OutputDevice/SystemWindow). It is even the default
to call these method(s) with a nullptr which means to repaint
all registered OutputDevices/SystemWindows. This is no
tooling intended to repaint parts of a View to 'any'
OutputDevice.
This is not very obvious and there are already other places
in the office that do that wrong and never got cleaned-up,
therefore also being fallbacks to not just let those cases
fail, but at least paint something (and give you a warning).
The caveat is that these fallbacks (e.g. in
SdrPageView::DrawLayer) have to use temporary SdrPaintWindow/
SdrPageWindow instances to make the paint work. Especially
when a temporary SdrPageWindow is used an evtl. existing
VC/VOC/OC structure will not be used and with it also not
the existing primitive decompositions.
I thought about bigger changes in PageView/PaintView/
PageWindow/PaintWindow area to sort that out, but this is
- despite being utterly necessary - too expensive for now,
keep a note about this needed change for the future.
For now I will just use an already (also for this purpose
already existing) 'compromize' that will do it for now.
Change-Id: I8c7ccee1deb0d69aad1d2145a7ac2039aca685b4
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/160155
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
It is not possible to use implCreateItemEntry/implCleanupItemEntry,
that is tooling limited *by purpose* to svl/Item/ItemSet stuff.
But what I can do is to do that SfxPoolItemHolder I already
talked/thought about. It is a helper that can safely hold a
SfxPoolItem in cases where an SfxItemSet is too expensive.
Think about it as a SfxItemSet for a single item. That solves
the problem why DirectPutItemInPool/DirectRemoveItemFromPool
is used in general (each usage is a 'compromize').
Did that now, works well. Editengine is now free of
DirectPutItemInPool/DirectRemoveItemFromPool.
Replaced ::CursorMoved with checkAndDeleteEmptyAttribs since all
these got static with no longer need to DirectRemoveItemFromPool.
Corrected create/delete counters.
Change-Id: Ia6e53f48ac2e479b461546515e68697039b5b628
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/159931
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
That allows to not create a local copy on the heap before
being able to check if a change is really necessary
Also added mfOrientation to Cell::operator==, it was missing. Maybe
with C++20 we should more use the default generated op== (or op<=>)
that may turn out to be more safe. For class Cell at least all members
(and sub-members of Style) are simple and simple comparable.
Change-Id: Idea2ef2abe68c4bb14aa776a8393ba5da92abd5c
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/159798
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
Removed some special stuff in the ItemSet/Item tooling regarding
ScPatternAttr. There are still quite a view (grep for
isExceptionalSCItem), but all these are identified and isolated.
Only for that Item (of over 500) are these exceptions, so that raelly
does not fit into the Item schemata in any way. Not even the pool
default of ScPatternAttr is without trouble. It is the only and last
Item that needs to be 'pooled' in the sense to avoid copies on any
price. That is OK, but should not be done in the ItemSet schemata.
In short: It should not be an Item at all. It should be changed
and renamed to something describing it's role (CellAttributes..?)
and get helped/assisted by something called CellAttributeHelper at
SC's model or Pool. It should not even be derived from SetItem, it
could just contain an shared_ptr of SfxItemSet (allows more and better
optimizations - think about Clone() and op==).
In parallel, all these hacks in the ItemSet/Item stuff could be
removed, making that faster and easier. Also quite some usages of
DirectPutItemInPool could be cleaned-up, this only exists since
there *is* no defined place to hold that data (coud be
CellAttributeHelper in the future). Putting Items directly to the
pool (and in most cases never remove them again) is just nonsense
and another hint that all this does not fit to the Item/ItemSet
schema at all.
This is now - after hard isolation of problems and have all working -
doable. It may be one of the next things to do, but there are
other candidates, too. Doing this would mostly only help SC...
Found another hack that uses DirectPutItemInPool and *never* removes
any Item again, see comments framelinkarray.cxx
And another one: PoolItemTest::testPool() explicitely tests
DirectPutItemInPool stuff - which makes no sense long-term,
but for now keep it and make it work by marking the slots used
as _bNeedsPoolRegistration == true
Have now overhauled the framelinkarray stuff to work without
DirectPutItemInPool and Cell not being a SfxPoolItem. That will
be much less complex and use much less calls/checks. Since this
is the data structure created for every calc repaint that should
get faster, too.
Also for now and memory loss security I added code to
DirectPutItemInPool to behave the same as with the unchanged
implCreateItemEntry: register Items so that the garbage collection
still is used. This will/can be removed when all usages of
DirectPutItemInPool is cleaned up.
Directly registering in DirectPutItemInPoolImpl is more tricky
than thought, but a good thing to do. Looking at that I saw that
tryRegisterSfxPoolItem is not used anymore, so rearranged some stuff.
Change-Id: If07762b0a25e2739027f81872441772dc82a25d9
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/159685
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
It may happen that DrawModel changes while a SlideShow is running,
e.g. from EditView/Scripts/UNO API. We can try to suppress these
as we already try with EditView in that mode. As an alternative
it is also possible to react to these changes in the SLideShow
in a useful manner.
This is the 2nd try, main change is to do all of this directly
in SlideShow, so no chechs/tests in common code are needed.
Change-Id: Iaa393d78af1fc40003c83d896508abe31cd790d5
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/159447
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
Due to the paradigm item change the test
make CppunitTest_sc_tablesheetobj
with CPPUNIT_TEST_NAME
sc_apitest::ScTableSheetObj::testSheetCellRangeProperties
got much slower. Unfortunately it did not break, so got unnoted.
I took a look now. First I intended to add some hashing in an
std::unordered_set using that hash values at ScPatternAttr, but
that is not even possible due to other data in that item that needs
to be compared. I had the impression that it was 'somehow' hashed
before, but after debugging the version before that change I
noted that also the list of existing items was linearly compared
to the new entry, using the operator==.
Thus the problem was not due to not hashing, but due to the
ScPatternAttr::operator==. That uses the hash (not changed),
but no longer finds equal entries.
This is because the hash code is made up from the SfxPoolItemPtrs
in the SfxItemSet, so when all are equal we can be sure the SfxItemSet
content is equal.
To use this the other way around is *not* correct: Even with
not all ptrs equal the SfxItemSets can still be equal, simply
by one SfxItemSet using another, but identical incarnation of
an item. Thuis means that ScPatternAttr::operator== does not
detect all cases of equality.
This worked in most cases before since most items were
'pooled' and thus much effort was used to ensure their uniqueness,
but even before the paradigm item change an item type could be
flagged as non-poolable. In that case, this already could fail
but with no too bad consequences (just one more copy of
an ScPatternAttr would stay).
So I fixed that mainly in adapting and optimizing
ScPatternAttr::operator==. The results are (same machine, same
compiler, dbg version, metioned test):
Version before item paradigm change:
user 0m50,778s
Version after item paradigm change:
user 20m13,944s
Version with memcmp:
user 0m48,845s
Version with hash:
user 0m48,710s
Since that hash does nothing else than to buffer the comparison of
those item pointers I just tried to use memcmp instead, as is already
used in other places (same file, ScPatternAttr::FastEqualPatternSets,
also SfxItemSet::operator==). As can be seen above it makes practically
no difference (memcomp even slightly faster).
Since that hash is only used in ScPatternAttr::operator== and is same
speed (memcomp linearly compares 56 SfxPoolItem ptrs) I decided to
remove it. It needs quite some spaces to be reset and re-calculated
which are not needed anymore. The calculation is based on dividing
the number of items by 4, so we are good with 56, but if someone has/
will adapt the items used by ScPatternAttr it is easy to forget to
adapt this, and not easy to change the alghorithm when it's not a
multiple of 4.
I also optimized/overhauled SfxItemSet::operator== (or better: the
SfxItemSet::Equals used by it). It is now better readable, too.
I also adapted ScAttrArray::AddCondFormat to not always incarnate/
delete ScPatternAttr instances, only when needed. This also helps
a bit and could be done in more places.
All in all it is really necessary to cleanup SC's usage of
ScPatternAttr - there are quite some possibilities to make that
cleaner and faster. In principle it's a huge 'compromize' to use
item functionailty to have a possibly 'cheap' maximum shared
SfxItemSet at a Cell.
Decided to make SfxItemSet::operator== even faster for the case
of unequal ranges by iterating over ranges of local SfxItemSet
and incremented offset. Still accesses to 2nd SfxItemSet will be
the expensive ones using the iterated WhichID.
Added two more things to SfxItemSet::operator==: We can return
early when both have no items set. For the unequal-ranges compare
I added an early-exit when Count() items were compared.
Looked at the errors that indeed do trigger the assert in
ScPatternAttr::operator== and hint to incarnations of ScPatternAttr
that do not use the needed range ATTR_PATTERN_START, ATTR_PATTERN_END.
Hunted down to come from ScViewFunc::ApplyAttributes and there from
some Dialogs, so seems some SC dialogs do not work with the correct
range schema for that item.
I tried code in ScViewFunc::ApplyAttributes to fix it, that works. I
also tried to hunt that down to the Dialogs that use the wrong schema
(TotalCount @SfxItemSet is 61 BTW). While it would be possible to do
so, it's no guarentee to have this fixed.
Thus I looked at ScPatternAttr::ScPatternAttr and added correciton
code when one with the wrong range schema gets created, this is
luckily not often needed and transfers the existing items with low
costs.
Maybe we should add a warning there if used, so at least new
implementations of stuff or old ones (the Dialogs) can be corrected?
Change-Id: I31da73ae30786bd6c5a08a5e3b7df8fe279af261
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/159592
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
Using the described steps in the bug report it is possible
to have an already disposed incarnation of ChildrenManagerImpl
that gets called with notifyEvent. If exposed it is necessary
not to execute actions, but to inform the caller by triggering
a lang::DisposedException(). The SfxBaseModel::notifyEvent
that executes this will then remove the instance from it's list.
Change-Id: Ie39e37d6e55ea66f441e732b612774b18d7c3ca1
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/159306
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
To understand this, some look back in history will be needed to see
why it is as it is today. In some (reworked) comments 'poolable' is
described as flag to hold Items in the ItemPool, also always having
only one incarnation of each possible Item.
This is not the original intention, but a side-effect. The reason is
what the binary format in the office did: To save a document, the
Objects & the Pool were saved, *not* individual Items *together*
with the objects. The Pool was completely (binary) saved (and loaded)
in one run.
Temporary IDs were used to represent at the objects in file which
Items were referenced. This *required* to have only one incarnation
per item to have a minimal binary file size, thus this high effort
was put into this. At doc load, the pool was loaded, all Items were
set to RefCount 5000, the references from the objects were restored
and then for each Item the RefCount was lowered by 5000 again
and - if being zero - deleted. Items for UI were marked 'non-poolable'
to *not* safe them with the document, so poolable was a flag to decide
if that Info/Item was to be saved with the document - or more direct:
if it is Model Data.
Items are small, so if we prefer runtime it is okay to no longer being
strict with this, anyways does not happen often and has only marginal
memory effects - compared to runtime effects/savings.
Other problems which this caused: One example is that objects in the
UNDO stack were still in the pool, so e.g. deleted pictures were saved
with the document despite no longer being used (!). That is the reason
we have an UndoItemPool and a method MigrateItemPool to move stuff to
that Pool when objects go to the UNDO stack - all of this is also no
longer needed.
Cleaning this up means to ideally have all items in the SfxItemSet,
no longer at the Pool. The Pool should be reduced to a 'Default-Item-
Holder' and a 'Slot-to-whichId-mapper'.
This needs thorough cleanups/removals, but will be worth it because
that massive simplification(s) will increase safety an runtime and make
migrating to the goal of completely type-based ItemSet stuff easier for
the future. Hopefully only view code in the office working with items
will have to be changed for this.
In this 1st step I already found that some 'compromizes' will be
needed:
- There are still Items that have to be at the pool to make the
Surrogate-stuff working. This gives back all Items in a Pool of a type
and is used in ca. 80 cases. Each one looks at these Items *without*
context (e.g. a SfxItemSet at an Object would be a context), so if e.g.
a dialog is open that temporarily uses Items of that type you would
also get these - without knowing about it...
To make that work there is still a mechanism to have Items at the Pool,
but now just *registering* (and un-reg) them without any sort/search/
remove needs. Also only for Items that need that, so I evaluated the
GetItemSurrogates calls and added some asserts when GetItemSurrogates
tries to access an unregistered item type which needs to be added.
- Another caveat is that there are about 250 places that directly put
Items to the Pool (not all remove these, that is done at pool deletion,
so some kind of silent 'garbage-collection' is in place). To have an
overview I renamed the accessing methods to separate them from the same
functionality at the SfxItemSet, which had the same names. An
implementation does still add these directly to the pool, there is no
way to cleanup those usages for now. In principle all these should be
changed to hold the data at an SfxItemSet.
I am still hunting problems. But you can build the office, all apps
work (including chart) and you can do speed comparisons already.
There are test throwing errors, so I hunt these now. It is hard to
give an estimation about how much more changes/corrections will be
needed.
Completed adaptions to new registered Items at Pool, that reduces the
failing tests. Still many that I need to hunt.
Added stuff to work around that 'compromize' in ScDocumentPool: It
overloads ::PutImpl of the pool to implement special handling for
a single Item in SC, the ScPatternAttr. In former code that method
was used from SfxItemSet and ::PutImpl at the pool directly, so it
was only used in one place. I am not sure if it was used from
the SfxItemSet functionality, but better offer it for now. To not
waste too much runtime the callbacks depend on the boolean
'NewItemCallback' at the SfxPoolItem, it gets set for that single
Item in SC and only then the callbacks trigger. I hope to get rid
of those again, e.g. newItem_UseDirect is only needed since we have
no 'real' StaticPoolDefaults currently - another thing that needs to
be cleaned up in a next step.
Since usages of impl(Create|Cleanup)ItemEntry and
Direct(Put|Remove)ItemInPoolImpl got more and more similar I decided to
unify that: move impl(Create|Cleanup)ItemEntry to tooling, make it
globally available in svl and use it also directly for
Direct(Put|Remove)ItemInPoolImpl. This slightly increases the failing
tests again, but only since in Direct(Put|Remove)ItemInPoolImpl that
fallback (e.g. tryToGetEqualItem) was used before, thus this is the
same class of errors (SfxPoolItem ptr-compare) as the others which I
will need to find anyways. Also fixed some missing stuff.
Have now idenified and redirected all SfxPoolItem ptr-compares
to be able to debug these - one cause for the remaining errors is
probably that before with bPoolable those often were sufficient, but
are no longer. Used the [loplugin:itemcompare] and a local clang
build to do so, see https://gerrit.libreoffice.org/c/core/+/157172
Stabilized Direct(Put|Remove)ItemInPoolImpl forwards, added parameter
to implCreateItemEntry to signal that it gets called from DirectPool
stuff - currently needed. Hopefully when getting rid of that DirectPool
stuff we can remove that again
Added two more debug functionalities:
- Added a SerialNumber to allow targeted debugging for deterministic
cases
- Added registering & listing of still-allocated SfxPoolItems at
office shutdown
Found PtrComp error in thints.cxx - POC, thanks to
areSfxPoolItemPtrsEqual. Will hopefully help more with other tests
Found some wrong asserts/warnings where I was too careful and not
finding something/succeeding is OK, fixes some UnitTests for SC
For SC I now just tried to replace all areSfxPoolItemPtrsEqual with
the full-ptr-content compare SfxPoolItem::areSame. I also needed to
experiment/adapt the newItem_Callback solution but got it working.
Did that replacement now for SW too, found some places where the
direct ptr compare is OK.
Continued for the rest of occurrences, now all 160 places evaluated.
Also done some cleanups.
Massive cleanups of stuff no longer needed with this paradigm change.
Also decided to keep tryToGetEqualItem/ITEM_CLASSIC_MODE for now.
It is used for *one* Item (ScPatternAttr/ATTR_PATTERN) in SC that
already needs many exceptions. Also useful for testing if errors
come up on this change to test if it is related to this.
Added forwarding of target Pool for ::Clone in SvxSetItem and
SvxSetItem, simplified SfxStateCache::SetState_Impl and returned
to simple ptr compares in SfxPoolItem::areSame to not do the test
in areSfxPoolItemPtrsEqual.
Debugged through UITest_calc_tests9 and found that in tdf133629
where BoxStyle is applied to fully selected empty calc the Item-
reuse fallback has to be used not only for ATTR_PATTERN, see
comment @implCreateItemEntry. Maybe more...
Problem with test_tdf156611_insert_hyperlink_like_excel. Found that
in ScEditShell::GetFirstURLFieldFromCell the correct SvxURLField
is found and returned as ptr, but it's usage crashes. That is due to
the SfxItemSet aEditSet used there gets destroyed at function return
what again deletes the SvxFieldItem that is holding the SvxURLField
that gets returned.
This shows a more general problem: There is no 'SfxPoolItemHolder'
that safely holds a single SfxPoolItem - like a SfxItemSet for a
single Item (if Items would be shared_ptrs, that would be a safe
return value).
That will be needed in the future, but for now use another solution:
Since I see no reason why EE_FEATURE_FIELD should not be shareable
I wil change this for ow in the SfxItemInfo for EditCharAttribField.
That way the Item returned will be shared (RefCnt > 1) and thus not
be deleted.
I changed the return value for GetURLField() and
GetFirstURLFieldFromCell() in ScEditShell: At least for
GetFirstURLFieldFromCell the return type/value was not safe: The
SvxFieldItem accessed there and held in the local temporary
SfxItemSet may be deleted with it, so return value can be
corrupted/deleted. To avoid that, return a Clone of SvxFieldData
as a unique_ptr.
With all that UnitTest debugging and hunting and to get the paradigm
change working to no longer rely on shared/pooled items I lost a
little bit focus on speed, so I made an optimization round for the
two central methods implCreateItemEntry/implCleanupItemEntry to
get back to the speed improvements that I detected when starting this
change. It was mainly lost due to that 'strange' chained pool stuff
we have, so I added to detect the target pool (the one at which the
WhichID is registered) directly and only once. Next thing to cleanup
will/should be the pool and it's concept, all this is not needed
and really costs runtime.
Since implCreateItemEntry/implCleanupItemEntry are executed millions
of times, each cycle counts here.
Had an error in the last changes: pool::*_Impl methods use index
instead of WhichID - most of them. Another bad trap, I really need
to cleanup pool stuff next.
Change-Id: I6295f332325b33268ec396ed46f8d0a1026e2d69
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/157559
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
...that use internally an sw::BroadcastingModify*. This caused
an error/crash with the testfile ooo58307-1.sxw, thanks to
Caolan to find it. For BG info please see comments in
the changed sw/source/core/attr/swatrset.cxx
Change-Id: Ia91fff19ffb39873c7e2bd5ff8806b95fc5ac7ba
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/157383
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
Added a counter for SfxItemSet usages, similar to the
already added one for SfxPoolItems to allow quick info
about evtl. problems/drawbacks of changes.
Replaced enum SfxItemKind in favour of settable boolean flags
directly at SfxPoolItem. These are organized as bitfield, do
not need more space as the enum and allow to be set separately
and multiple ones at the same time.
Flags for PoolDefault/StaticDefault/DeleteOnIdle use this now
and are quickly accessible booleans. It is not a problem that
theoretically the flags for PoolDefault/StaticDefault could now
both be set - this is internal to SfxItem stuff and not accessible
from normal code, so can be managed.
Added for debug build a bitfield boolean m_bDeleted
that will be set in the SfxPoolItem destructor. Of course
it's usability will depend on the freed space not yet being
re-used, but will hopefully help in the debugger to detect
reasons for failure (would have helped at least me before).
Added for replacement of virtual method IsVoidItem() another
bitfield bool m_bIsVoidItem that is set in the constructors
of SfxVoidItem. Also had to add some constructors to do that
which were defaulted before. This is mainly because the base
class SfxPoolItem does *not* have a copy-constructor that
copies the members (flags/RefCnt/WhichID) and we should keep
that 'indirect reset' when Cloning. isVoidItem() is now a simple
boolean member access - the bitfield does the needed masking.
This spares one entry in the virtual function table of
SfxPoolItem which is derived more than 500 times.
Used the results of the experiment at
https://gerrit.libreoffice.org/c/core/+/156774 to change
some accesses to IsVoidItem() to use SfxItemState instead.
This is for preparation of splitting up the two usages of
SfxVoidItems, see commit text in the experiment.
If this shows problems in the future those six places documented
there may have to be changed back to use the new isVoidItem(),
but should also check the ptr this time to be non-zero.
Removed SFX_ITEMS_SPECIAL that is no more used anywhere.
Change-Id: Ib687ca2362d72a4651c75aee0c67029088f68947
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/156805
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
Again this is a change to improve understandabilty/changeability
of SfxItemSet code plus some cleanups. Still did a callgrind round
to check - it slightly got faster. In a start/load(complex SW doc)/
show/shutdown cycle compared with master I get 96.722 mio cycles
compared with 99.851 mio in master.
Main focus was to isolate two aspects: preparation and cleanup of
an Item for usage in an SfxItemSet. For that we now have
implCreateItemEntry: to do all needed actions to create/prepare
an Item for membership, including evtl. AddRef/Cloning/using
ItemPool stuff.
implCleanupItemEntry: to do all needed actios to correctly
clean that Item up again.
All formally accesses distributed over SfxItemSet (and other places)
are cleaned-up to use these. The Item-counter in DBG code that I
already added helped a lot to do this.
Also cleaned up PutImpl to 1st check if action is necessary (Item is
already in place) or not, reducing spot to cleanup an Item that
was handed over as bPassingOwnership to one place.
I also added a 2nd flag, bItemIsSetMember, that tells if the Item
given as input is already member of an SfxItemSet, in that case a
shortcut can be used (increase AddRef, use).
Adapted all places AFAP to use the new container interface
(begin(), end(), ...) where useful.
Made GetItemState inline and directly use the tooling method. Same
for InvalidateItem.
Added much more comments to describe what's going on or to hint at
problems (check for CAUTION).
Removed PutDirect - not needed anymore, probably was there to not get
recursive death loop with callbacks in SW.
More smaller changes.
Checked with all apps, played around. Still, stuff may come up, so
I put on gerrit the tests will show and give further hints. At last
SfxItemSet is a minefield :-)
Had to adapt SfxItemSet::implCreateItemEntry when input Item is
a StaticDefaultItem. SfxItemPool::PutImpl needs to be called in
that case.
Had to correct bItemIsSetMember in all cases if the transfer
of SfxPoolItems is between SfxItemSets with different
SfxItemPools. This is and will be necessary as long as the
Items are stored at the pool.
After a hard deep-dive I found the error: m_nCount was not in
all cases correct, invalid items get counted.
Win build *insists* for initialzation of local var aEntry in
SfxItemSet::PutImpl, triggers warning C4701:
"potentially uninitialized local variable 'aEntry' used". This
is not the case here, but I know of no way to silence the
warning in another way, so added an extra-call to begin().
Re-added to use static pool defaults directly, possible After
the fix 6d8c6e8d60, thanks for
that. This avoids some cloning of Items.
CAUTION: static default items are not *that* static as it seems
(or: should be). If they are freed with the Pool (see
::ReleaseDefaults) they will be deleted. If the target pool is
different from the source pool static default items from the
source pool can be used that then might be deleted (sigh).
A solution would be to change all pools to really work on
static instances of default items.
Another one would be to know here that the
targetPool != sourcePool, so maybe extend
bItemIsSetMember -> bSamePool.
A good test for this is CppunitTest_chart2_uichart/testTdf98690.
Until solving/cleaning up we unfortunately *have* to continue
to clone static default items...
Change-Id: Ibd8dc6d612f594a5ad88c75fcee8726d89a6090c
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/156306
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
Huge offsets of GridOffset(s) are a hint for error -> usually the
conditions for calculation have changed. E.g. - I saw errors with
+/-5740, that was in the environment of massive external UNO API
using LO as target.
If condtions for this calculation change, it is usually required to
call
- ViewObjectContact::resetGridOffset(), or
- ObjectContact::resetAllGridOffsets() or
- ScDrawView::resetGridOffsetsForAllSdrPageViews()
as it is done e.g. when zoom changes (see ScDrawView::RecalcScale()).
Theoretically these resets have to be done for any precondition
changed that is used in the calculation of that value (see
ScDrawView::calculateGridOffsetForSdrObject).
This is not complete and would be hard to do so. Since it is just a
buffered value and re-calculation is not expensive (linear O(n)) we
can just reset suspicious values here what fixes the problem.
Hopefully - when that non-linear ViewTransformation problem for the
calc-view gets solved one day - all this can be removed again. For
now, let's just reset here and force re-calculation.
Added a SAL_WARN to inform about this, too.
Change-Id: I09137f7703fd00c2351a288a40bf87a2691ba6e3
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/155983
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
This change is not about speed improvements but diverse
preparations to make changes/reading/understanding easier.
It does not change speed AFAIK.
Added a global static debug-only counter to allow getting
an overview over number of all allocated SfxPoolItem's
and the still alloated ones at office shutdown. The values
are used in Application::~Application to make a short info
statement. It allows to be able to quickly detect if an
error in future changes may lead to memory losses - these
would show in dramaitically higher numbers then (hopefully)
immediately.
Moved SfxVoidItem to own source/header.
Added container library interface support to SfxItemSet,
adapted already some methods to use it - not all possible,
I will commit & get status from gerrit 1st if all still works
and then continue.
Changed INVALID_POOL_ITEM from -1 to use a global unique
incarnation of an isolated derivation from SfxPoolItem. It
allows to avoid the (-1) pointer hack. Since still just
pointers are compared it's not worse. NOTE: That way, more
'special' SfxPoolItem's may be used for more States - a
candidate is e.g. SfxVoidItem(0) which represents ::DISABLED
state -- unfortunately not only, it is also used (mainly for
UI stuff) with 'real' WhichIDs - hard to sort out, will have
to stay that way for now AFAIK.
Changed INVALID_POOL_ITEM stuff to use a static extern
incarnated item in combination with a inline method
to return it, called GetGlobalStaticInvalidItemInstance().
Isolated create/cleanup of a SfxPoolItem entry in
SfxItemSet to further modularize/simplify that. It is
currently from constructor & destructor but already shows
that PoolDefaults are handled differently - probably an
error. Still, for now, do no change in behaviour (yet).
Got regular 'killed by the Kill-Wrapper' messages from
gerrit, seems to have to do with UITest_sw_findReplace.
That python/c++ scripting stuff is hard to debug, but
finally I identified the problem has to do with
the INVALID_POOL_ITEM change. It was in
SfxItemSet::InvalidateAllItems() where still a (-1)
was used -> chaos in detecting invalid items.
Change-Id: I595e1f25ab660c35c4f2d19c233d1dfadfe25214
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/155675
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
When browsing cachegrind data I stumbled over the notification
callback used by Writer in SfxItemSet::Changed. That is a
virtual method that gets called in quite some places to forward
item changes, SW uses it to record these.
For that purpose always (quite some) data gets prepared without
checking if this is necessary, this uses calls to ::Get and
::GetDefaultItem to have either the old or new Item from the
parent or default (pool).
This is not needed - except for Writer. Even there this
mechanism is not always active. Thus I:
- removed SfxItemSet::Changed, replaced with a settable callback
member of type std::function<...>. Thus one less virtual function
and depenence in SfxItemSet
- added a callback functor to SwAttrSet that can be set at the
SfxItemSet it is derived from
- setting/releasing this only in used cases. It is not even used
all the time in SW.
- moved the creation/processing of needed data to a member
function in SW (SwAttrSet::changeCallback). All processing and
evtl. needed data is now created there - on demand.
- adapted all places in SfxItemSet where that mechanism is used
to only call it if set & without pre-calculating anything
- since all former calls/usages were pretty similar I could put
all of this to SwAttrSet::changeCallback
This leads to use that only when needed now. Naturally, SW will
potentially profit less than the other apps.
Here are callgrind numbers with this change using OfficeStart,
DocLoad, DocClose, OfficeShutdown. This change also has potential
avantages @runtime/UI which also did all preparations to call
SfxItemSet::Changed all the time:
Writer doc: 0,9907 ~1%
old: 93842 mio
new: 92971 mio
Draw/Impress doc: 0,9971 ~2,8%
old: 170023 mio
new: 169544 mio
::Get reduces from 1416103 to 293874 calls
::GetDefaultItem reduces from 2252336 to 1927209 calls (nearly half)
Calc doc: 0.9868 ~1,3%
old: 194708 mio
new: 192130 mio
::Get reduces from 882298 to 880087 calls
::GetDefaultItem reduces from 4611901 to 2633555 calls (nearly half)
Of course this highly depends on the used test documents, so it can
only be a excerpt result.
Also adapted SfxItemSet::MergeRange a little bit: Do nothing not only
when a single new slot is already contaioned, but check if all slots
are already present. This works well and fast using the formally added
caching mechanism.
Change-Id: I4d369d2e5b21aa7a21687177518150515e3de954
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/155559
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
I checked for was to speedup SfxItemSet stuff, so had
(besides other things) a look at WhichRangesContainer
and it's usage(s). Problem with the WhichRanges is that
a WhichID which you try to find is usually inside that
range, so binary search is no option. You have to detect
in which range the WhichID is hosted and can the directly
calculate the index into the array of Items at the
SfxtemSet.
Currently when needing to transform a WhichID to an index
into the array of Items in SfxItemSet the array of the
WhichRangesContainer is searched linearly from the start
every time.
This can be a little bit speed up by buffering the last
successful 'hit' and trying to re-use it. Also the
special case of a single WhichPair (e.g. UI stuff) is
worth having a look.
All acesses to that transformation are changed to use
the tooling method getOffsetFromWhich() at the
WhichRangesContainer which does the transformation.
This also needed cleanup of ItemOffsetHint instances
& stuff around it. It does not more than before but
also profits from the single entry buffer.
I added some DBG_UTIL-based stuff to watch the hit/miss
ratio, which is heavily changing from app to app & usage,
but varies around 1.5 to 3.5, also saw 6.5 and more at
document import.
NOTE: I already checked if sorting the WhichPair(s) in
WhichRangesContainer by their 'width' (highest WhichID
mnius lowest WhichID helps. The idea was when the Items
would be used in a regular manner that when having the
widest WhichPairs at the start, the buffer would even
be better used - but doing tests in all apps shows
nearly no gain, so I left that out.
NOTE: Not too much speedup, but faster...
Had to deep-debug due to CppunitTest_sw_odfexport failing,
found a slight diff between GetItemState impls, corrected.
Also added more changes, e.g. TotalCount is now a member
to not always have to calculate it from the
WhichRangesContainer.
Extended GetWhichByPos to 1st try to find a set Item, else
iterate over WhichRangesContainer.
This is due to SfxItemIter's implementations of
GetItemState and ClearItem which both up to now just
accessed the SfxPoolItem array of the SfxItemSet, ignoring
that no Item (nullptr) or state DONTCARE (-1) may have been
set there (no item is prevented by ite Iterator, but better
be careful).
Added WhichRangesContainer::getWhichFromOffset and made
SfxItemSet::GetWhichByOffset use it. Addedd optimizations
there for single-entry WhichPair and using the buffer at
WhichRangesContainer which is possible.
Removed debug comparing stuff (had a test that used the
former adapted GetItemStateImpl method in
SfxItemSet::GetItemState and compared with the changed
GetItemState_ForWhichID).
Added some comments and assertions where useful.
Made ClearSingleItem_ForOffset work without handing
over WhichID, that makes calls using it simpler and
avoids calculating the WhichID just for that call.
Change-Id: I54de552368b654f00f115978715f8241eb603752
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/155316
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
