Background: I do experiment actively with a much simplified, type-safe ersion of ItemSet/ItemPool to make it easier for everyone to deal with these, still keeping performance and mem footprint. Stumbled over SfxItemState::SfxItemState::UNKNOWN which is in the way for that simplifications. Looking for a way around it relealed that it might bne hard and take off quite some of the 'easier' portion, so I'll try to remove it's usages as much as possible. Im most cases these usages (127) are about checking if that Item is 'known' at the SfxItemSet which uses the info of the defined ranges at these - mostly in UI stuff (anyways a 2nd ItemSet use that is kinda 'hacky') to hide controls. I will try to replace usages with what really is intended and protocol it here. This might lead to some changes in the core that hopefully will do no harm, but to be able to revert thede and maybe cancel that try here again will not all here
Grepping immediately
1st commit is: cf4e87469baf13fb2766d0f2593fcc2b9b33bc9b
Armin Le Grand committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/cf4e87469baf13fb2766d0f2593fcc2b9b33bc9b tdf#130428 SfxItemState::UNKNOWN replacements It will be available in 7.0.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Julien Nabet committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/9811796aba7360fc5b7230a8b314a56fbf6ab27a Related tdf#130428: let's add some asserts It will be available in 7.0.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
(In reply to Commit Notification from comment #4) > Julien Nabet committed a patch related to this issue. > It has been pushed to "master": > > https://git.libreoffice.org/core/commit/ > 9811796aba7360fc5b7230a8b314a56fbf6ab27a > > Related tdf#130428: let's add some asserts > And these asserts can now be triggered in report designer. Just insert a label, and use menu Format > Character...
Julien, Maxim: Thanks to you both, adding asserts and getting hins that and HOW these get triggered is what I was hoping for! Will take a 2nd look...
> And these asserts can now be triggered in report designer. Just insert a > label, and use menu Format > Character... Huh - could not find '...use menu Format > Character...' -> I have no Menu 'Format' and also not in context menu. Can you please describe where exactly you get that menu?
(In reply to Armin Le Grand from comment #7) > Huh - could not find '...use menu Format > Character...' -> I have no Menu > 'Format' and also not in context menu. Can you please describe where exactly > you get that menu? Open a database (or create a new one). In the left sidebar switch to "Reports", and click on "Create Report in Design View...". This will open the report builder, which has a Format > Character... menu item.
(In reply to Maxim Monastirsky from comment #8) Thanks, can reproduce now. Indeed, a AlignmentTabPage is opened, but AlignmentTabPage::GetRanges() is *not* called - sigh. That whole rptui::openCharDialog constructs it's own ItemPool/ItemSet stuff, including *own* WhichID's starting at XATTR_FILL_LAST. These do not correspond with the WHichID's in cui\source\tabpages\align.cxx *at all*, e.g for the Item SvxHorJustifyItem it uses it's own WhichID ITEMID_HORJUSTIFY which is XATTR_FILL_LAST + 26 -> 1033 + 26 -> 1059 which gets accessed in the TabPage using SID_ATTR_ALIGN_HOR_JUSTIFY which expands to ( ((10000 + 1499) + 1) + 71 ) -> something completely different. Mapping seems to be done in SfxTabPage::GetWhich which maps to sal_uInt16 GetWhich( sal_uInt16 nSlot, bool bDeep = true ) const { return pSet->GetPool()->GetWhich( nSlot, bDeep ); } SfxItemPool::GetWhich again uses pItemInfos which get set at 4th param in SfxItemPool constructor using aItemInfos which maps { SID_ATTR_ALIGN_HOR_JUSTIFY, true } So - what does sal_uInt16 nWhich = GetWhich(SID_ATTR_ALIGN_HOR_JUSTIFY); in cui\source\tabpages\align.cxx do...? mnStart/mnEnd are XATTR_FILL_FIRST,ITEMID_WEIGHT_COMPLEX, thus it gets XATTR_FILL_FIRST + the offset of the found SID_ATTR_ALIGN_HOR_JUSTIFY, so 46. XATTR_FILL_FIRST is 1014, XATTR_FILL_LAST is 1033, #define ITEMID_VERJUSTIFY XATTR_FILL_LAST + 27 -> indeed leads to the local ITEMID_VERJUSTIFY,l despite being defined based on XATTR_FILL_LAST, not on XATTR_FILL_FIRST. This is all very tricky and highly depends on these offsets - sigh. That's exactly what I want to get rid of in the ling term to make things easier - but no chance as long as this stuff is mixed as this... The crash is due to SID_ATTR_ALIGN_STACKED (which is ( SID_SVX_START + 229 )) -> 10229, is indeed *not* defined in aItemInfos for the temporary ItemPool/ItemSet in reportdesign\source\ui\misc\UITools.cxx, thus produces state UNKNOWN.
In short: My assumption (hope?) that AlignmentTabPage::GetRange() will always be used is wrong. There are four calls to AddTabPage("alignment", RID_SVXPAGE_ALIGNMENT.* of which three do *not* use a 'TabRangesFunc'-ptr. Thus I have to revert that change and continue thinking about possibilities...
Armin Le Grand (Collabora) committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/cfb9e6d9976fd7e87fc9605ca79264df041744ee Revert "Related tdf#130428: let's add some asserts" It will be available in 7.0.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Dear Armin Le Grand, This bug has been in ASSIGNED status for more than 3 months without any activity. Resetting it to NEW. Please assign it back to yourself if you're still working on this.
Back to assigned...
Remove SfxItemState::READONLY now in f3b737ab76efaf1a70dfb22c6b60b08b340cf343 and b60a9bec769339f5f0fe008ef4985263d0cf689e, continuing with checking (slowly) usages of SfxItemState::UNKNOWN
Armin Le Grand (Allotropia) committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/bcc28948d0fd7b4f005a13db923c3e853d74817c tdf#130428 remove unnecessary usage of SfxItemState::UNKNOWN It will be available in 7.2.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Armin Le Grand (Allotropia) committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/e11c3740845959ab25f9b644681c97a7ded24d1c tdf#130428 remove unnecessary usage of SfxItemState::UNKNOWN It will be available in 7.2.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Armin Le Grand (Allotropia) committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/a292fb847dade1912d281adc44c92a9ba3e8b61d tdf#130428 remove unnecessary usage of SfxItemState::UNKNOWN It will be available in 7.2.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
After these commits we are down to 105 grep-hits - and the 'simple' cases are mainly done...
Back to the basic problem: SfxItemState::UNKNOWN defines a check (see SfxItemSet::GetItemState) if the WhichID is defined in the array of Which Ranges (m_pWhichRanges) of the SfxItemSet. The GetWhich call e.g. in AlignmentTabPage uses SfxTabPage::GetWhich: sal_uInt16 GetWhich( sal_uInt16 nSlot, bool bDeep = true ) const { return pSet->GetPool()->GetWhich( nSlot, bDeep ); } which lands at SfxItemPool::GetWhich sal_uInt16 GetWhich( sal_uInt16 nSlot, bool bDeep = true ) const; so this WhichID-replacement usually is bDeep == true which means the check is if in any of the Which Ranges of the hierarchy of SfxItemPools. This is not very important, we want to get rid of the WhichID-mapping, so do not touch that at all. More important is the GetItemState call that usually also goes deep: SfxItemState GetItemState( sal_uInt16 nWhich, bool bSrchInParent = true, const SfxPoolItem **ppItem = nullptr ) const; with bSrchInParent = true being the default. To replace this in some way we definitely will need a representation of some state like 'explicitely defined for usage in this context' at the SfxItemSet - or better, at it's future replacement.
Thus, the Item derivation classes that get used with SfxItemState::UNKNOWN will in some way need to be extra-qualified at the SfxItemSet. It will not really be possible to find all places where the SfxItemSets for explicitely e.g. the TabPages like AlignmentTabPage will be created - this is also just to unsafe. For a reliable way to do that we need to identify all usages of the involved WhichIDs and where these get registered at the SfxItemSets, including the nowadays possible merging of those WhichID ranges. The problem here is to identify all those places, since not really all singe WhichIDs are given, but ranges of those - so no ID to reliably grep for. Maybe some clang extension may help here that can trigger... Have to ask someone who knows better about that than I do.
These are the SlotIDs which are checked against SfxItemState::UNKNOWN, I tried to buil Groups (maybe an option to sign functional groups). The ones with TAB in front are double: Group AlignmentTabPage SID_ATTR_ALIGN_STACKED SID_ATTR_ALIGN_ASIANVERTICAL SID_ATTR_ALIGN_LINEBREAK SID_ATTR_ALIGN_HYPHENATION SID_ATTR_ALIGN_SHRINKTOFIT SID_ATTR_ALIGN_HOR_JUSTIFY SID_ATTR_ALIGN_INDENT SID_ATTR_ALIGN_VER_JUSTIFY SID_ATTR_ALIGN_DEGREES SID_ATTR_ALIGN_LOCKPOS SID_ATTR_FRAMEDIRECTION Group SvxBorderTabPage SID_ATTR_BORDER_DIAG_TLBR SID_ATTR_BORDER_DIAG_BLTR SID_ATTR_ALIGN_MARGIN Group SvxCharNamePage SID_ATTR_CHAR_LANGUAGE SID_ATTR_CHAR_CJK_LANGUAGE SID_ATTR_CHAR_CTL_LANGUAGE SID_ATTR_CHAR_COLOR SID_ATTR_CHAR_WORDLINEMODE SID_ATTR_CHAR_EMPHASISMARK SID_ATTR_CHAR_CASEMAP SID_ATTR_CHAR_RELIEF SID_ATTR_CHAR_CONTOUR SID_ATTR_CHAR_SHADOWED SID_ATTR_CHAR_HIDDEN SID_ATTR_CHAR_ROTATED SID_ATTR_CHAR_WIDTH_FIT_TO_LINE Group SvxPageDescPage SID_ATTR_FRAMEDIRECTION SID_ATTR_FRAMEDIRECTION Group SvxExtParagraphTabPage SID_ATTR_PARA_PAGENUM SID_ATTR_PARA_FORBIDDEN_RULES SID_ATTR_PARA_HANGPUNCTUATION SID_ATTR_PARA_SCRIPTSPACE Group ScDrawShell SID_ENABLE_HYPHENATION Group ScDrawTextObjectBar SID_HYPERLINK_GETLINK SID_OPEN_HYPERLINK SID_EDIT_HYPERLINK SID_COPY_HYPERLINK_LOCATION SID_REMOVE_HYPERLINK SID_TRANSLITERATE_HALFWIDTH SID_TRANSLITERATE_FULLWIDTH SID_TRANSLITERATE_HIRAGANA SID_TRANSLITERATE_KATAKANA SID_ENABLE_HYPHENATION SID_THES EE_CHAR_FONTINFO EE_CHAR_FONTHEIGHT EE_CHAR_WEIGHT EE_CHAR_ITALIC ScCellShell SID_CLIPBOARD_FORMAT_ITEMS ScEditShell EE_CHAR_FONTINFO EE_CHAR_FONTHEIGHT EE_CHAR_WEIGHT EE_CHAR_ITALIC ScFormatShell ATTR_FONT ATTR_FONT_HEIGHT ATTR_FONT_WEIGHT ATTR_FONT_POSTURE ATTR_BORDER ATTR_BORDER_INNER SdModule SID_OPENDOC SID_OPENHYPERLINK DrawDocShell SID_RELOAD SlideSorterController SID_RELOAD DrawViewShell SID_RELOAD SwGrfExtPage RES_GRFATR_MIRRORGRF RES_FRAMEDIR SwNumPositionTabPage FN_PARAM_NUM_PRESET SwTextGridPage RES_FRAMEDIR SfxToSwPageDescAttr SID_ATTR_PARA_PAGENUM
Found some more doable cases, down to 88 ocurrences
Armin Le Grand (Allotropia) committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/5c3ad59612697cf0afee0f836e9ee77f0920356b tdf#130428 remove unnecessary usage of SfxItemState::UNKNOWN It will be available in 7.2.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Armin Le Grand (Allotropia) committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/552ceeb5c6ccf2573adfd092cc4b2ce214a9c2df tdf#130428 remove unnecessary usage of SfxItemState::UNKNOWN It will be available in 7.2.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Armin Le Grand (Allotropia) committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/2baf6f654244a67715e4b3d86c49144ce6b5dd13 tdf#130428 remove unnecessary usage of SfxItemState::UNKNOWN It will be available in 7.2.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Armin Le Grand (Allotropia) committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/64378ded7b87fa785f13b998e006d87f44a2776d tdf#130428 remove unnecessary usage of SfxItemState::UNKNOWN It will be available in 7.2.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Armin Le Grand (Allotropia) committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/eb0222e80ba6f5b7b11f1e43f3723162beb2ba71 tdf#130428 remove unnecessary usage of SfxItemState::UNKNOWN It will be available in 7.2.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Armin Le Grand (Allotropia) committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/3b46c3068af26d6be65cfe309c751e310a22d129 tdf#130428 remove unnecessary usage of SfxItemState::UNKNOWN It will be available in 7.2.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
SfxItemState::UNKNOWN is also used with controller items, e.g. see SfxControllerItem::GetItemState: SfxItemState::UNKNOWN Enabled, but no further status information available. Typical for <Slot>s, which anyway are sometimes disabled, but otherwise do not change their appearance. This gets used when pState->IsVoidItem() && !pState->Which() which means: (a) that the SfxPoolItem is a SfxVoidItem (SfxVoidItem::IsVoidItem() const { return true; }), normally used (see SfxItemSet::GetItemState) for SfxItemState::DISABLED (b) that the 0 == WhichID That SfxItemState::UNKNOWN is feeded to ::StateChanged calls (52 implementations) for all kinds of controls. Also used in SfxStateCache as local var eLastState to buffer against eState. Usually this looks like (60 defines in *.hxx) // Old sfx2 interface virtual void StateChanged( sal_uInt16 nSID, SfxItemState eState, const SfxPoolItem* pState ); where sal_uInt16 nSID -> is the SlotID SfxItemState eState -> is as described in SfxControllerItem::GetItemState const SfxPoolItem* pState -> is the SfxPoolItem For the current SfxItemState::UNKNOWN this gets: nSID -> exists, identifies a slot eState -> SfxItemState::UNKNOWN pState -> SfxVoidItem(0) Thus there is *no* data from an SfxPoolItem, only the SlotID. Not sure (yet) how to replace this, but maybe SfxItemState::DONTCARE Enabled but there were only ambiguous values available (i.e. non that can be queried). would fit well. There cannot really be useful usages in the ::StateChanged methods for this - need to check... BTW: That description is double, can be found in SfxToolBoxControl::GetItemState and in SfxControllerItem::GetItemState. AND there is also SfxStatusListener::statusChanged where pItem.reset(new SfxVoidItem( m_nSlotID )); eState = SfxItemState::UNKNOWN; is used - in CONTRAST to defines above != SfxVoidItem(0), but also feed to a StateChanged( eState, pItem.get() ); that does *not* take a SlotID -> maybe another hack which needs to be solved...? Both ::GetItemState mentioned above will in tis case return SfxItemState::DEFAULT instead of SfxItemState::UNKNOWN -> is that WANTED?
Checked for StateChanged: (1) SfxToolBoxControl: virtual void StateChanged( sal_uInt16 nSID, SfxItemState eState, const SfxPoolItem* pState ); (2) SfxStatusListener: virtual void StateChanged( SfxItemState eState, const SfxPoolItem* pState ); (3) SfxStatusBarControl: virtual void StateChanged( sal_uInt16 nSID, SfxItemState eState, const SfxPoolItem* pState ); (4) Control, vcl::Window: virtual void StateChanged( StateChangedType nStateChange ); (1,2,3) are used for StateChange with SfxItemState parameter. To be able to better check/read/analyze these I propose a NameChange to allow direct grepping/searching. (1,3) even have same parameter signature, (2) avoids this by 'handing' over nSID inside the SfxItem (if available - as mentioned in comment 29 this leads to use 'hacks' to hand in a SfxVoidItem(nSID) as parameter transport (4) s not interesting in this case - just vcl::Window handling using StateChangedType, no coincidence with SfxItemState at all. -> Rename (1,2,3) -> Keep (4), do not touch -> for now, ignore hacks at (2), do not extend parameters/signature
Armin Le Grand (Allotropia) committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/04cd6749177f886f382e8bcd026f95112ee22473 tdf#130428 remove unnecessary usage of SfxItemState::UNKNOWN It will be available in 7.3.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.