Re: [PUSHED 3.6] was vcl checkbox behaviourRe: vcl checkbox behaviour
Noel Power píše v Pá 06. 07. 2012 v 14:41 +0100: > On 28/06/12 19:02, Jan Holesovsky wrote: > I ask > for a review here. > > the changes in question are : > http://cgit.freedesktop.org/libreoffice/core/commit/?id=a1345cd93a57ec7d9352f2c71ec2664332ce5e76 > http://cgit.freedesktop.org/libreoffice/core/commit/?id=2bc2d09bba2e3f34e8ce13571de9ff7007e1c2b6 The change makes sense. The patches look reasonable. It works. I checked few things in the UI and did not find any side effects. It seems that you did lots checks to do not break things => pushed into 3-6 branch: http://cgit.freedesktop.org/libreoffice/core/commit/?h=libreoffice-3-6&id=99cad9fd7f413adecffc527d334eff616799cc4b http://cgit.freedesktop.org/libreoffice/core/commit/?h=libreoffice-3-6&id=aea425a8cd851e6bee02247673c8996b58789484 Best Regards, Petr ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: REVIEW[3.6] was vcl checkbox behaviourRe: vcl checkbox behaviour
On 28/06/12 19:02, Jan Holesovsky wrote: Hi Noel, Noel Power píše v Čt 28. 06. 2012 v 13:36 +0100: Actually the "for quite a while" spans to 8ab086b6cc054501bfbf7ef6fa509c393691e860, the inital import of button.cxx to CVS :-( - it has been imported in 2000 with this already in place. Looking deeper, I thought that the 'alignment of checkboxes without text' behavior might make sense for the dialogs like Tools -> Options -> Load/Save -> Microsoft Office and alike, where you'd want to center the checkbox; but it seems to me that it is actually SvLBoxButton that is used there, not CheckBox. So - can you in any way check if we are actually using this CheckBox feature anywhere in the UI? [Ie. if we construct a CheckBox without text, but with WB_CENTER or WB_RIGHT?] If we do not use it, I'd get rid of that [even in UNO controls and everywhere], as you proposed. Regards, Kendy I dug deeper, in the end I could only find CheckBoxControl http://opengrok.libreoffice.org/xref/core/svtools/inc/svtools/editbrowsebox.hxx#291 which has a member pBox which is a TriStateBox ( which inherits from CheckBox ) that has no label and is aligned with WB_CENTER, This class is used quite abit in dbaccess and also in one place in sw ( and indeed depends on the behaviour mentioned ) For the moment I changed both radiobutton and checkbox as described, I introduced a new virtual to allow a subclass to modify the behaviour ( I already did that in CheckBoxControl ) I did this for both radiobutton and checkbox even though I didn't see any instances of RadioButton that use this functionality. I think the only was to see if I missed something is to commit the changes ( which I did to master ) However I think there is a better chance of 3.6 highlighting some missed usecase, with that in mind I ask for a review here. ( note I am away for the next two weeks so if someone would approve this I would ask they commit it too :-) ) the changes in question are : http://cgit.freedesktop.org/libreoffice/core/commit/?id=a1345cd93a57ec7d9352f2c71ec2664332ce5e76 http://cgit.freedesktop.org/libreoffice/core/commit/?id=2bc2d09bba2e3f34e8ce13571de9ff7007e1c2b6 going forward I want to change both of these controls to accept some new parameter that would allow the position/alignment of the image within its bounding area separate from WB_LEFT, WB_RIGHT & WB_CENTER which now only apply to the label ( if it exists ) Noel ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: vcl checkbox behaviour
On 29/06/12 10:09, Noel Power wrote: On 28/06/12 19:02, Jan Holesovsky wrote: [...] Actually the "for quite a while" spans to 8ab086b6cc054501bfbf7ef6fa509c393691e860, the inital import of button.cxx to CVS :-( - it has been imported in 2000 with this already in place. Looking deeper, I thought that the 'alignment of checkboxes without text' behavior might make sense for the dialogs like Tools -> Options -> Load/Save -> Microsoft Office and alike, where you'd want to center the checkbox; sure and any such instances could be covered by setting the bounding box of the checkbox appropriately I guess but it seems to me that it is actually SvLBoxButton that is used there, not CheckBox. so no issue there ( but don't forget radio buttons afaict are also treated the same way and probably would make sense to change them also ) So - can you in any way check if we are actually using this CheckBox feature anywhere in the UI? [Ie. if we construct a CheckBox without text, but with WB_CENTER or WB_RIGHT?] If we do not use it, I'd get rid of that [even in UNO controls and everywhere], as you proposed. Regards, Kendy yes, I will try to check ( hopefully without missing anything ) :-) I would also like to make an enhancement to really control the alignment of the control itself e.g. either left or right. This would not affect the behaviour previously described ( which is more about the justification of the label ) but the position of the label relative to the control e.g. - control aligned left: [x] some label - control aligned right: some label [x] So, I spent quite some time today trolling through the source code, I could only find one place where either RadioButton or CheckBox had either WB_LEFT, WB_RIGHT or WB_CENTER set for the control ( and where there was no label ) It actually isn't even a vcl CheckBox but a TriStateBox that has WB_CENTER applied and then only in the specific case of the CheckBoxControl http://opengrok.libreoffice.org/xref/core/svtools/inc/svtools/editbrowsebox.hxx#291 This class is used quite abit in dbaccess and also in one place in sw. So, I am going to take the plunge and remove this behaviour. Of course I am also going to try and modify CheckBoxControl to do the right thing without depending on this strange behaviour ( and cross fingers etc. that I didn't miss some usage of the weird 'feature' ) So.. basically last chance if you know of some reason why we really want to preserve the present behaviour. Noel ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: vcl checkbox behaviour
On 28/06/12 19:02, Jan Holesovsky wrote: [...] Actually the "for quite a while" spans to 8ab086b6cc054501bfbf7ef6fa509c393691e860, the inital import of button.cxx to CVS :-( - it has been imported in 2000 with this already in place. Looking deeper, I thought that the 'alignment of checkboxes without text' behavior might make sense for the dialogs like Tools -> Options -> Load/Save -> Microsoft Office and alike, where you'd want to center the checkbox; sure and any such instances could be covered by setting the bounding box of the checkbox appropriately I guess but it seems to me that it is actually SvLBoxButton that is used there, not CheckBox. so no issue there ( but don't forget radio buttons afaict are also treated the same way and probably would make sense to change them also ) So - can you in any way check if we are actually using this CheckBox feature anywhere in the UI? [Ie. if we construct a CheckBox without text, but with WB_CENTER or WB_RIGHT?] If we do not use it, I'd get rid of that [even in UNO controls and everywhere], as you proposed. Regards, Kendy yes, I will try to check ( hopefully without missing anything ) :-) I would also like to make an enhancement to really control the alignment of the control itself e.g. either left or right. This would not affect the behaviour previously described ( which is more about the justification of the label ) but the position of the label relative to the control e.g. - control aligned left: [x] some label - control aligned right: some label [x] again such an enhancement is driven by interoperability but... one step at a time :-) Noel ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: vcl checkbox behaviour
Hi Noel, Noel Power píše v Čt 28. 06. 2012 v 13:36 +0100: > >> I am somewhat stumbling over what would be best to do, change the > >> underlying vcl class unconditionally ? ( that would imply the uno > >> controls behaviour also changes unconditionally ) or ... if we change > >> the underlying vcl class conditionally what about the uno controls?, > >> should we change them also conditionally? or unconditionally? It would > >> seem to me if there is some consensus that the 'normal' vcl behaviour is > >> not that normal but is 'abnormal' then I would be inclined to change the > >> vcl class unconditionally ( and similarly the behaviour of the dependant > >> uno controls ) Any thoughts ? > > So the behavior sound curious to me; but OTOH nearly sounds as if > > somebody did that deliberately. Do you have pointers to the code where > > does the "apply the Align property on the control itself" live? Any > > history that would be of some worth around there? > no like I mentioned earlier in the mail the behaviour is indeed > intentional see > http://opengrok.libreoffice.org/xref/core/vcl/source/control/button.cxx#3209 > However, I just don't understand why it, historically this behaviour > seems to be there for quite a while, the last change around there was > http://cgit.freedesktop.org/libreoffice/core/commit/?id=d727e1094312e34fedc2bfdb12b43c109a954054 > > which adds a new logic leg for checkboxes with images ( which is a > strange concept for me too :-) ) Actually the "for quite a while" spans to 8ab086b6cc054501bfbf7ef6fa509c393691e860, the inital import of button.cxx to CVS :-( - it has been imported in 2000 with this already in place. Looking deeper, I thought that the 'alignment of checkboxes without text' behavior might make sense for the dialogs like Tools -> Options -> Load/Save -> Microsoft Office and alike, where you'd want to center the checkbox; but it seems to me that it is actually SvLBoxButton that is used there, not CheckBox. So - can you in any way check if we are actually using this CheckBox feature anywhere in the UI? [Ie. if we construct a CheckBox without text, but with WB_CENTER or WB_RIGHT?] If we do not use it, I'd get rid of that [even in UNO controls and everywhere], as you proposed. Regards, Kendy ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: vcl checkbox behaviour
On 28/06/12 12:44, Jan Holesovsky wrote: Hi Noel, Noel Power píše v Po 25. 06. 2012 v 11:46 +0100: I am somewhat stumbling over what would be best to do, change the underlying vcl class unconditionally ? ( that would imply the uno controls behaviour also changes unconditionally ) or ... if we change the underlying vcl class conditionally what about the uno controls?, should we change them also conditionally? or unconditionally? It would seem to me if there is some consensus that the 'normal' vcl behaviour is not that normal but is 'abnormal' then I would be inclined to change the vcl class unconditionally ( and similarly the behaviour of the dependant uno controls ) Any thoughts ? So the behavior sound curious to me; but OTOH nearly sounds as if somebody did that deliberately. Do you have pointers to the code where does the "apply the Align property on the control itself" live? Any history that would be of some worth around there? no like I mentioned earlier in the mail the behaviour is indeed intentional see http://opengrok.libreoffice.org/xref/core/vcl/source/control/button.cxx#3209 However, I just don't understand why it, historically this behaviour seems to be there for quite a while, the last change around there was http://cgit.freedesktop.org/libreoffice/core/commit/?id=d727e1094312e34fedc2bfdb12b43c109a954054 which adds a new logic leg for checkboxes with images ( which is a strange concept for me too :-) ) ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: vcl checkbox behaviour
Hi Noel, Noel Power píše v Po 25. 06. 2012 v 11:46 +0100: > I am somewhat stumbling over what would be best to do, change the > underlying vcl class unconditionally ? ( that would imply the uno > controls behaviour also changes unconditionally ) or ... if we change > the underlying vcl class conditionally what about the uno controls?, > should we change them also conditionally? or unconditionally? It would > seem to me if there is some consensus that the 'normal' vcl behaviour is > not that normal but is 'abnormal' then I would be inclined to change the > vcl class unconditionally ( and similarly the behaviour of the dependant > uno controls ) Any thoughts ? So the behavior sound curious to me; but OTOH nearly sounds as if somebody did that deliberately. Do you have pointers to the code where does the "apply the Align property on the control itself" live? Any history that would be of some worth around there? Regards, Kendy ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: vcl checkbox behaviour
On 25/06/12 11:46, Noel Power wrote: I wonder is the behaviour that I see here normal or expected ( to me is isn't but I am not very ui-enabled ). Look at the attached jpeg, the first three checkboxes have the Align property set as Left, Center and Right, with no label the checkbox 'box' is aligned within the bounding rectangle of the control according to the 'Align' property. However, if there is a label then the checkbox 'box' is always left aligned and it is the text label that is aligned according to the 'Align' property. Now to my puny brain the latter behaviour ( as if there is a label present ) is what I would expect always. I would like to change the behaviour to be like that, but... the code is obviously and intentionally written to behave in the way described, does this make sense? would it be ok to change this? is it only me that things this is bizarre? For context this behaviour is causing me some grief when importing controls from mso formats, I could of course add some flag to the vcl Checkbox[*] class to make it behave like I wish conditionally, note: that also could require the toolbox awt ( and formcontrol ) uno checkbox control to support an extra flag also that could be persisted or their behaviour could independently be changed unconditionally ( and no need to support extra properties ). I am somewhat stumbling over what would be best to do, change the underlying vcl class unconditionally ? ( that would imply the uno controls behaviour also changes unconditionally ) or ... if we change the underlying vcl class conditionally what about the uno controls?, should we change them also conditionally? or unconditionally? It would seem to me if there is some consensus that the 'normal' vcl behaviour is not that normal but is 'abnormal' then I would be inclined to change the vcl class unconditionally ( and similarly the behaviour of the dependant uno controls ) Any thoughts ? Noel [*] same issue exists for (at least) for radio buttons no thoughts anyone ? ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
