Re: [dev] SfxItemSet assumptions and assertions
Hi Bjoern, Björn Michaelsen wrote, On 06/14/10 13:34: > Hi all, > > while testing the new SfxItemSets in cws new_itemsets I came across a > few interesting uses of itemsets which I had considered to be illegal > (but I might be wrong there). I assumed: > - non-void items should only be put at the same which id in a set as the > which id on the item itself. > - void items should always have a which id of 0 and are allowed to be > put anywhere. Who says they would need to have which ID 0 ? > > Here are a few examples where these assumptions fail: > > Opening an empty writer document for examples fires the assertion at: > http://hg.services.openoffice.org/cws/new_itemsets/file/5d3400d7452f/svl/inc/svl/itemset.hxx#l74 > > The backtrace shows the first call triggering this is coming in from > SwDoc::GetTxtCollFromPool(..), followed by calls from > SwFmt::SetFmtAttr(..). Is there any reason why a non-void, non-invalid > item is put at a different which id in the set than the which id of the > item itself? Sound like the road to perdition to me ... > > When typing in a number in a cell in calc the assertion at: > http://hg.services.openoffice.org/cws/new_itemsets/file/5d3400d7452f/svl/source/items/itemset.cxx#l98 > > The backtrace shows the call triggering this is coming in from > EditView::GetAttribs(..). Is there any reason why a void item should > have a which id > 0 set? And if yes, what is the reason for using such > semantics? E.g. how are a void item with which id 0 and a void item > with which id > 0 differing in behaviour? The EditEngine is using VoidItems for some attributes which are only markers. SW at least did the same, don't know if it still does. And because I mainly use which IDs when working with attributes, and not RTTI, they have non zero which IDs... Malte. > > Best Regards, > > Bjoern > > - To unsubscribe, e-mail: dev-unsubscr...@openoffice.org For additional commands, e-mail: dev-h...@openoffice.org
Re: [dev] SfxItemSet assumptions and assertions
Am Mon, 14 Jun 2010 15:11:57 +0200 schrieb Malte Timmermann : > Hi Bjoern, > > Björn Michaelsen wrote, On 06/14/10 13:34: > > Hi all, > > > > while testing the new SfxItemSets in cws new_itemsets I came across > > a few interesting uses of itemsets which I had considered to be > > illegal (but I might be wrong there). I assumed: > > - non-void items should only be put at the same which id in a set > > as the which id on the item itself. > > - void items should always have a which id of 0 and are allowed to > > be put anywhere. > > Who says they would need to have which ID 0 ? Well, as I said I can be wrong here. But a SfxVoidItem with which id set to 0 is interpreted as "disabled". However, some functions only test for the type being SfxVoidItem, while others test only for the which id being 0. Thus having a "half-disabled" item might lead to "interesting" results. See for example: http://svn.services.openoffice.org/opengrok/xref/DEV300_m82/svl/source/items/itemset.cxx#542 (SfxItemSet::GetItemState) http://svn.services.openoffice.org/opengrok/xref/DEV300_m82/svl/source/items/itemset.cxx#589 (SfxItemSet::Put) http://svn.services.openoffice.org/opengrok/xref/DEV300_m82/svl/source/items/itemset.cxx#2040 (SfxItemSet::DisableItem) So, when you used SfxVoidItems, did you intend them to be interpreted as "disabled"? Best Regards, Bjoern - To unsubscribe, e-mail: dev-unsubscr...@openoffice.org For additional commands, e-mail: dev-h...@openoffice.org
Re: [dev] SfxItemSet assumptions and assertions
Björn Michaelsen wrote, On 06/14/10 16:09: > Am Mon, 14 Jun 2010 15:11:57 +0200 > schrieb Malte Timmermann : > >> Hi Bjoern, >> >> Björn Michaelsen wrote, On 06/14/10 13:34: >>> Hi all, >>> >>> while testing the new SfxItemSets in cws new_itemsets I came across >>> a few interesting uses of itemsets which I had considered to be >>> illegal (but I might be wrong there). I assumed: >>> - non-void items should only be put at the same which id in a set >>> as the which id on the item itself. >>> - void items should always have a which id of 0 and are allowed to >>> be put anywhere. >> Who says they would need to have which ID 0 ? > > Well, as I said I can be wrong here. But a SfxVoidItem with which id > set to 0 is interpreted as "disabled". However, some functions only > test for the type being SfxVoidItem, while others test only for the > which id being 0. Thus having a "half-disabled" item might lead to > "interesting" results. See for example: > > http://svn.services.openoffice.org/opengrok/xref/DEV300_m82/svl/source/items/itemset.cxx#542 > (SfxItemSet::GetItemState) > http://svn.services.openoffice.org/opengrok/xref/DEV300_m82/svl/source/items/itemset.cxx#589 > (SfxItemSet::Put) > http://svn.services.openoffice.org/opengrok/xref/DEV300_m82/svl/source/items/itemset.cxx#2040 > (SfxItemSet::DisableItem) > > So, when you used SfxVoidItems, did you intend them to be interpreted > as "disabled"? No - but it doesn't matter, because I only use them for character attributes (single items), not for paragraph attributes (item set). They probably only get into the item sets because the SfxItemPool has default items, I don't think that I put them in some SfxItemSet explicitly. Of course they get into the SfxItemPool... BTW - they are also used in the binary document format, SfxItemSet load/store, so I also don't know what would happen here if the IDs where 0. Malte. > > Best Regards, > > Bjoern > > > - > To unsubscribe, e-mail: dev-unsubscr...@openoffice.org > For additional commands, e-mail: dev-h...@openoffice.org > - To unsubscribe, e-mail: dev-unsubscr...@openoffice.org For additional commands, e-mail: dev-h...@openoffice.org
Re: [dev] SfxItemSet assumptions and assertions
On 14.06.2010 17:49, Malte Timmermann wrote: Björn Michaelsen wrote, On 06/14/10 16:09: Am Mon, 14 Jun 2010 15:11:57 +0200 schrieb Malte Timmermann: Hi Bjoern, Björn Michaelsen wrote, On 06/14/10 13:34: Hi all, while testing the new SfxItemSets in cws new_itemsets I came across a few interesting uses of itemsets which I had considered to be illegal (but I might be wrong there). I assumed: - non-void items should only be put at the same which id in a set as the which id on the item itself. - void items should always have a which id of 0 and are allowed to be put anywhere. Who says they would need to have which ID 0 ? Well, as I said I can be wrong here. But a SfxVoidItem with which id set to 0 is interpreted as "disabled". However, some functions only test for the type being SfxVoidItem, while others test only for the which id being 0. Thus having a "half-disabled" item might lead to "interesting" results. See for example: http://svn.services.openoffice.org/opengrok/xref/DEV300_m82/svl/source/items/itemset.cxx#542 (SfxItemSet::GetItemState) http://svn.services.openoffice.org/opengrok/xref/DEV300_m82/svl/source/items/itemset.cxx#589 (SfxItemSet::Put) http://svn.services.openoffice.org/opengrok/xref/DEV300_m82/svl/source/items/itemset.cxx#2040 (SfxItemSet::DisableItem) So, when you used SfxVoidItems, did you intend them to be interpreted as "disabled"? No - but it doesn't matter, because I only use them for character attributes (single items), not for paragraph attributes (item set). They probably only get into the item sets because the SfxItemPool has default items, I don't think that I put them in some SfxItemSet explicitly. So this is code broken (the code in the ItemSet, not yours). If some code in SfxItemSet just checks for SfxVoidItems to decide whether it is a disabled item, while other code just checks for ID == 0, the problem is already there. It's also an unnecessary complexity to require the check of two attributes (type *and* ID). The next interesting question is what should happen if items derived from SfxVoidItems are used ... So perhaps coding "disabled" as SfxVoidItem is a severe design flaw. So it would be better if disabling (and checking for disabed state) should be possible only by using explicit methods from SfxItemSet. If the implementation requires to code that by storing a special item, this item should be a private class in SfxItemSet so that it can't be used outside. The problem with that approach is that it requires a non-trivial amount of work. :-( Of course they get into the SfxItemPool... BTW - they are also used in the binary document format, SfxItemSet load/store, so I also don't know what would happen here if the IDs where 0. So we are lucky that this format is doomed. :-) Regards, Mathias -- Mathias Bauer (mba) - Project Lead OpenOffice.org Writer OpenOffice.org Engineering at Sun: http://blogs.sun.com/GullFOSS Please don't reply to "nospamfor...@gmx.de". I use it for the OOo lists and only rarely read other mails sent to it. - To unsubscribe, e-mail: dev-unsubscr...@openoffice.org For additional commands, e-mail: dev-h...@openoffice.org
Re: [dev] SfxItemSet assumptions and assertions
Am Tue, 15 Jun 2010 09:04:45 +0200 schrieb Mathias Bauer : > So this is code broken (the code in the ItemSet, not yours). I would argue that both code is broken, because as is the ItemSet has no well-defined contract to design against. Of course, Malte is no more guilty than anybody else as the only alternative is not using the broken ItemSet -- which I assume was not an option. > If some code in SfxItemSet just checks for SfxVoidItems to decide > whether it is a disabled item, while other code just checks for ID == > 0, the problem is already there. It's also an unnecessary complexity > to require the check of two attributes (type *and* ID). The next > interesting question is what should happen if items derived from > SfxVoidItems are used ... > > So perhaps coding "disabled" as SfxVoidItem is a severe design flaw. Yes, it is. > So it would be better if disabling (and checking for disabed state) > should be possible only by using explicit methods from SfxItemSet. If > the implementation requires to code that by storing a special item, > this item should be a private class in SfxItemSet so that it can't be > used outside. The problem with that approach is that it requires a > non-trivial amount of work. :-( Well, the implementation in new_itemsets only checks for the which id being 0 and never checks for the type being an SfxVoidItem (outside assertions). So an disabled item is consistently defined as any item returning an which id of 0 in the new itemset implementation. Now the interesting part is finding the clients of the itemset that rely on void items being interpreted as "disabled" by the itemset and fixing those Best Regards, Bjoern - To unsubscribe, e-mail: dev-unsubscr...@openoffice.org For additional commands, e-mail: dev-h...@openoffice.org
Re: [dev] SfxItemSet assumptions and assertions
On 15.06.2010 11:47, Björn Michaelsen wrote: Well, the implementation in new_itemsets only checks for the which id being 0 and never checks for the type being an SfxVoidItem (outside assertions). So an disabled item is consistently defined as any item returning an which id of 0 in the new itemset implementation. That's an improvement and it allows to use the "API" consistently. OTOH it is still ugly. :-) I consider the way how the ItemSet codes the "disabled" status an implementation detail. Now the interesting part is finding the clients of the itemset that rely on void items being interpreted as "disabled" by the itemset and fixing those Question is if that is more effort then replacing all code that puts items with ID = 0 with code calling "DisableItem" instead. That would make putting an item with ID=0 an invalid action. BTW: I don't remember if any of the two ItemSet implementations returns an VoidItem in a GetItem call for a disabled item or if NULL is returned then. IMHO it always should be the latter. Back to your primary topic. If we identify the places where a VoidItem with ID != 0 is put, and if the developers state that this is by intent, we can remove that assertion and make putting VoidItems with ID != 0 a valid action that does not disable that item. The question why ItemSet::Put must allow that nWhich differs from the item's which is still open. I assume that it's related to different pools having different WhichIds for the same item. But knowing it for sure would perhaps allow us to define a less fuzzy interface. Regards, Mathias -- Mathias Bauer (mba) - Project Lead OpenOffice.org Writer OpenOffice.org Engineering at Sun: http://blogs.sun.com/GullFOSS Please don't reply to "nospamfor...@gmx.de". I use it for the OOo lists and only rarely read other mails sent to it. - To unsubscribe, e-mail: dev-unsubscr...@openoffice.org For additional commands, e-mail: dev-h...@openoffice.org
Re: [dev] SfxItemSet assumptions and assertions
Mathias Bauer wrote, On 06/16/10 10:11:
> On 15.06.2010 11:47, Björn Michaelsen wrote:
>
>> Well, the implementation in new_itemsets only checks for the which id
>> being 0 and never checks for the type being an SfxVoidItem (outside
>> assertions). So an disabled item is consistently defined as any item
>> returning an which id of 0 in the new itemset implementation.
>
> That's an improvement and it allows to use the "API" consistently. OTOH
> it is still ugly. :-) I consider the way how the ItemSet codes the
> "disabled" status an implementation detail.
>
>> Now the interesting part is finding the clients of the itemset that
>> rely on void items being interpreted as "disabled" by the itemset and
>> fixing those
> Question is if that is more effort then replacing all code that puts
> items with ID = 0 with code calling "DisableItem" instead. That would
> make putting an item with ID=0 an invalid action.
>
> BTW: I don't remember if any of the two ItemSet implementations returns
> an VoidItem in a GetItem call for a disabled item or if NULL is returned
> then. IMHO it always should be the latter.
We have
const SfxPoolItem& Get()
and
const SfxPoolItem* GetItem(...)
Not sure why we have two different Get Methods, but the first doesn't
allow to return NULL, so I assume the second one would also have to
return the same VoidItem for consistency.
>
> Back to your primary topic. If we identify the places where a VoidItem
> with ID != 0 is put, and if the developers state that this is by intent,
> we can remove that assertion and make putting VoidItems with ID != 0 a
> valid action that does not disable that item.
Yes, I guess it would be better to keep allowing VoidItems with Which !=
0. Or we search for all places where we might have != 0 VoidItems.
Places I found:
EditEngine has some, but that could be changed.
I have no clue here:
http://svn.services.openoffice.org/opengrok/xref/DEV300_m81/sfx2/source/control/sfxstatuslistener.cxx#233
if ( pType == ::getVoidCppuType() )
232 {
233 pItem = new SfxVoidItem( m_nSlotID );
234 eState = SFX_ITEM_UNKNOWN;
235 }
I don't think the slot id would be zero...
But this looks like we really would mix up slot and which ids.
Some more such things in sfx2/source/control/
SW:
rReq.SetReturnValue(SfxVoidItem(bLabel ? FN_LABEL : FN_BUSINESS_CARD));
rSet.Put( SfxVoidItem( SID_ATTR_ZOOMSLIDER ) );
rReq.SetReturnValue( SfxVoidItem( rReq.GetSlot() ) );
SVX:
new SfxVoidItem(SDRATTR_SHADOW3D );
new SfxVoidItem(SDRATTR_SHADOWPERSP );
I didn't check the [all...] links in OpenGrok, but with these hits I
already feel that we simply should keep the old behavior...
>
> The question why ItemSet::Put must allow that nWhich differs from the
> item's which is still open. I assume that it's related to different
> pools having different WhichIds for the same item. But knowing it for
> sure would perhaps allow us to define a less fuzzy interface.
Putting them on a different ID than it's own ID doesn't sound right to me.
For the different which ID stuff we have the SlotID mechanism.
But that would probably need cloning the item and changing the which
when putting it into a pool/itemset where different which ids are use.
So the situations where we put items on different IDs should be
investigated more closely.
But again, looking at
pItem = new SfxVoidItem( m_nSlotID );
rReq.SetReturnValue( SfxVoidItem( rReq.GetSlot() ) );
it seems to be a supported and wanted scenario.
Malte.
>
> Regards,
> Mathias
>
-
To unsubscribe, e-mail: dev-unsubscr...@openoffice.org
For additional commands, e-mail: dev-h...@openoffice.org
Re: [dev] SfxItemSet assumptions and assertions
And now that I just read Bjoerns last mail wrt old/current behavior:
At least for the EditEngine I am quite sure these items have been in
some ItemPool, but never in an ItemSet.
So that might be the reason why I never had an issue with that.
Not sure what would happen with the VoidItems listed below in other places.
Malte.
Malte Timmermann wrote, On 06/16/10 11:07:
>
> Mathias Bauer wrote, On 06/16/10 10:11:
>> On 15.06.2010 11:47, Björn Michaelsen wrote:
>>
>>> Well, the implementation in new_itemsets only checks for the which id
>>> being 0 and never checks for the type being an SfxVoidItem (outside
>>> assertions). So an disabled item is consistently defined as any item
>>> returning an which id of 0 in the new itemset implementation.
>> That's an improvement and it allows to use the "API" consistently. OTOH
>> it is still ugly. :-) I consider the way how the ItemSet codes the
>> "disabled" status an implementation detail.
>>
>>> Now the interesting part is finding the clients of the itemset that
>>> rely on void items being interpreted as "disabled" by the itemset and
>>> fixing those
>> Question is if that is more effort then replacing all code that puts
>> items with ID = 0 with code calling "DisableItem" instead. That would
>> make putting an item with ID=0 an invalid action.
>>
>> BTW: I don't remember if any of the two ItemSet implementations returns
>> an VoidItem in a GetItem call for a disabled item or if NULL is returned
>> then. IMHO it always should be the latter.
>
> We have
> const SfxPoolItem& Get()
> and
> const SfxPoolItem* GetItem(...)
>
> Not sure why we have two different Get Methods, but the first doesn't
> allow to return NULL, so I assume the second one would also have to
> return the same VoidItem for consistency.
>
>> Back to your primary topic. If we identify the places where a VoidItem
>> with ID != 0 is put, and if the developers state that this is by intent,
>> we can remove that assertion and make putting VoidItems with ID != 0 a
>> valid action that does not disable that item.
>
> Yes, I guess it would be better to keep allowing VoidItems with Which !=
> 0. Or we search for all places where we might have != 0 VoidItems.
>
> Places I found:
> EditEngine has some, but that could be changed.
>
> I have no clue here:
> http://svn.services.openoffice.org/opengrok/xref/DEV300_m81/sfx2/source/control/sfxstatuslistener.cxx#233
> if ( pType == ::getVoidCppuType() )
> 232 {
> 233 pItem = new SfxVoidItem( m_nSlotID );
> 234 eState = SFX_ITEM_UNKNOWN;
> 235 }
>
> I don't think the slot id would be zero...
> But this looks like we really would mix up slot and which ids.
> Some more such things in sfx2/source/control/
>
> SW:
> rReq.SetReturnValue(SfxVoidItem(bLabel ? FN_LABEL : FN_BUSINESS_CARD));
> rSet.Put( SfxVoidItem( SID_ATTR_ZOOMSLIDER ) );
> rReq.SetReturnValue( SfxVoidItem( rReq.GetSlot() ) );
>
> SVX:
> new SfxVoidItem(SDRATTR_SHADOW3D );
> new SfxVoidItem(SDRATTR_SHADOWPERSP );
>
> I didn't check the [all...] links in OpenGrok, but with these hits I
> already feel that we simply should keep the old behavior...
>
>> The question why ItemSet::Put must allow that nWhich differs from the
>> item's which is still open. I assume that it's related to different
>> pools having different WhichIds for the same item. But knowing it for
>> sure would perhaps allow us to define a less fuzzy interface.
>
> Putting them on a different ID than it's own ID doesn't sound right to me.
> For the different which ID stuff we have the SlotID mechanism.
> But that would probably need cloning the item and changing the which
> when putting it into a pool/itemset where different which ids are use.
> So the situations where we put items on different IDs should be
> investigated more closely.
>
> But again, looking at
> pItem = new SfxVoidItem( m_nSlotID );
> rReq.SetReturnValue( SfxVoidItem( rReq.GetSlot() ) );
> it seems to be a supported and wanted scenario.
>
> Malte.
>
>> Regards,
>> Mathias
>>
>
> -
> To unsubscribe, e-mail: dev-unsubscr...@openoffice.org
> For additional commands, e-mail: dev-h...@openoffice.org
>
-
To unsubscribe, e-mail: dev-unsubscr...@openoffice.org
For additional commands, e-mail: dev-h...@openoffice.org
Re: [dev] SfxItemSet assumptions and assertions
On 16.06.2010 11:07, Malte Timmermann wrote: We have const SfxPoolItem& Get() and const SfxPoolItem* GetItem(...) Not sure why we have two different Get Methods, but the first doesn't allow to return NULL, so I assume the second one would also have to return the same VoidItem for consistency. I assume that Get() can't be used right away because the internally stored item pointer may have the value NULL or even -1(!). So I think that you can't call GetItem() before checking if the ItemSet has an item and it would be just wrong if it would be called for an invalid item. Putting them on a different ID than it's own ID doesn't sound right to me. For the different which ID stuff we have the SlotID mechanism. But that would probably need cloning the item and changing the which when putting it into a pool/itemset where different which ids are use. So the situations where we put items on different IDs should be investigated more closely. But again, looking at pItem = new SfxVoidItem( m_nSlotID ); rReq.SetReturnValue( SfxVoidItem( rReq.GetSlot() ) ); it seems to be a supported and wanted scenario. The fact that it is used that not mean that it is correct or even necessary. ;-) The question is what is the intent of writing this code and if that really requires such a fuzzy interface. Regards, Mathias -- Mathias Bauer (mba) - Project Lead OpenOffice.org Writer OpenOffice.org Engineering at Sun: http://blogs.sun.com/GullFOSS Please don't reply to "nospamfor...@gmx.de". I use it for the OOo lists and only rarely read other mails sent to it. - To unsubscribe, e-mail: dev-unsubscr...@openoffice.org For additional commands, e-mail: dev-h...@openoffice.org
