[PATCH]fdo#55846 Comboboxes werent displayed in vertical toolbars
Eike Rathke
erack at redhat.com
Fri May 17 13:33:09 PDT 2013
Hi Janit,
On Wednesday, 2013-04-24 00:51:59 +0530, Janit Anjaria wrote:
> I am hereby submitting my patch.
Sorry, that was lingering on the list too long. I'd also suggest that
you don't explicitly Cc a developer when submitting a patch on the
mailing list if s/he didn't ask for it. It leaves the impression to
others reading the list that the dev on Cc would be responsible. Which
I am not ;-)
> diff --git a/vcl/source/window/toolbox.cxx b/vcl/source/window/toolbox.cxx
> @@ -239,16 +239,16 @@ void ToolBox::ImplCalcBorder( WindowAlign eAlign, long& rLeft, long& rTop,
>
> if ( eAlign == WINDOWALIGN_TOP )
> {
> - rLeft = borderwidth+dragwidth;
> + rLeft = borderwidth+dragwidth-150;
How did you derive the hardcoded value 150, where does it originate? To
me it seems to be a value that by chance matches your screen,
resolution, window size, current width of pane, and maybe more. It does
not seem to be a generally applicable value. On the other hand, if it
really was, a named constant value would be much better suited than
repeating the hard coded value in several places throughout the code.
> else if ( eAlign == WINDOWALIGN_LEFT )
> {
> - rLeft = borderwidth;
> + rLeft = borderwidth-120;
Same here, 120 what and why?
> - rRight = 0;
> + rRight = 15;
And this 15?
> @@ -730,7 +730,7 @@ Size ToolBox::ImplCalcSize( const ToolBox* pThis, sal_uInt16 nCalcLines, sal_uIn
> else if ( nCalcMode == TB_CALCMODE_VERT )
> {
> pThis->mpData->mbAssumeDocked = sal_True; // force non-floating mode during calculation
> - ImplCalcBorder( WINDOWALIGN_LEFT, nLeft, nTop, nRight, nBottom, pThis );
> + ImplCalcBorder( WINDOWALIGN_LEFT, nLeft, nTop, nRight, nBottom, pThis );
Please don't screw up existing indentation if it is reasonable.
> - aSize.Width() = nCalcLines * pThis->mnMaxItemWidth;
> + aSize.Width() = ((nCalcLines) * ((pThis->mnMaxItemWidth)));
The extra parentheses here only make the code harder to read and are not
needed.
> @@ -1919,7 +1919,8 @@ sal_Bool ToolBox::ImplCalcItem()
> if( it->mbVisibleText && !mbHorz )
> {
> long tmp = it->maItemSize.Width();
> - it->maItemSize.Width() = it->maItemSize.Height();
> + //fdo#55846 The UI for the same is managed by changing the snippet here.
> + it->maItemSize.Width() = it->maItemSize.Height() + 150;
The 150 again.
And please align comments with the code.
The wording of the comment is not really helpful. It only states that
something is done but does not explain. "the UI is managed by changing
the snippet" doesn't mean anything to me. Comments, if necessary, should
explain why something is done in the code, or clarify the non-obvious.
> @@ -1967,13 +1968,15 @@ sal_Bool ToolBox::ImplCalcItem()
> // it is only required for docked toolbars
>
> long nFixedWidth = nDefWidth+nDropDownArrowWidth;
> - long nFixedHeight = nDefHeight;
> +
> + long nFixedHeight = nDefHeight;
Again, please don't screw up existing indentation if it is reasonable.
> else
> - nMaxWidth = nFixedWidth;
> + //fdo#55846(for maintaining and checking the UI the constant is added)
> + nMaxWidth = nFixedWidth + 250;
Why is it the value 250?
And please align comments with the code.
Again, "for maintaining and checking the UI the constant is added" is
not meaningful. Maintain and check? What? Where? How?
> @@ -2355,7 +2358,7 @@ void ToolBox::ImplFormat( sal_Bool bResize )
> if ( mnWinStyle & WB_BORDER )
> {
> nTop = TB_BORDER_OFFSET1 + mnTopBorder;
> - nLeft = TB_BORDER_OFFSET2 + mnLeftBorder;
> + nLeft = TB_BORDER_OFFSET1 + mnLeftBorder;
Why change to TB_BORDER_OFFSET1 instead of TB_BORDER_OFFSET2 ?
> @@ -2372,7 +2375,7 @@ void ToolBox::ImplFormat( sal_Bool bResize )
> {
> long nWinWidth = mnDX - nLeft - nRight;
> if( nWinWidth > nLineSize )
> - nLineSize = nWinWidth;
> + nLineSize = nWinWidth;
Again, please don't screw up existing indentation if it is reasonable.
> @@ -2580,9 +2583,9 @@ void ToolBox::ImplFormat( sal_Bool bResize )
> }
> else
> {
> - it->maCalcRect.Left() = nX+(nLineSize-aCurrentItemSize.Width())/2;
> + it->maCalcRect.Left() = nX+(nLineSize-aCurrentItemSize.Width());
I have doubts about this. According to the comment in the source the
intention is to center the rectangle, whereas now it is right aligned.
> it->maCalcRect.Top() = nY;
> - it->maCalcRect.Right() = it->maCalcRect.Left()+aCurrentItemSize.Width()-1;
> + it->maCalcRect.Right() = it->maCalcRect.Left()+aCurrentItemSize.Width();
Not subtracting 1 anymore probably means that the right side of the
rectangle now may be drawn over or under some other element or border,
which does not look correct to me.
> diff --git a/vcl/source/window/toolbox2.cxx b/vcl/source/window/toolbox2.cxx
> @@ -256,21 +256,20 @@ Size ImplToolItem::GetSize( sal_Bool bHorz, sal_Bool bCheckMaxWidth, long maxWid
> {
> Size aSize( rDefaultSize ); // the size of 'standard' toolbox items
> // non-standard items are eg windows or buttons with text
> -
> + mbShowWindow=sal_True;
Unconditionaly setting mbShowWindow to true for ALL cases does not seem
to be correct.
> if ( (meType == TOOLBOXITEM_BUTTON) || (meType == TOOLBOXITEM_SPACE) )
> {
> aSize = maItemSize;
>
> - if ( mpWindow && bHorz )
> + if (mpWindow)
Removing the check for bHorz here ...
> {
> // get size of item window and check if it fits
> // no windows in vertical toolbars (the default is mbShowWindow=sal_False)
Note that it says "no windows in vertical toolbars (the default is
mbShowWindow=sal_False)" which you just changed above.
> Size aWinSize = mpWindow->GetSizePixel();
> - if ( !bCheckMaxWidth || (aWinSize.Width() <= maxWidth) )
> + if ( !bCheckMaxWidth || (aWinSize.Width() <= maxWidth))
... and only checking for width here and not checking for height in the
bHorz case looks wrong.
> {
> aSize.Width() = aWinSize.Width();
> aSize.Height() = aWinSize.Height();
> - mbShowWindow = sal_True;
Considering the new default (true) this would be correct, but ...
> }
> else
> {
> @@ -278,7 +277,7 @@ Size ImplToolItem::GetSize( sal_Bool bHorz, sal_Bool bCheckMaxWidth, long maxWid
> {
> aSize.Width() = 0;
> aSize.Height() = 0;
... this would need to set mbShowWindow=false then.
However, I think the default should not be changed but instead the
necessary checks introduced to handle the !bHorz case properly.
> - }
> + }
Again, please don't screw up existing indentation if it is reasonable.
> @@ -286,7 +285,7 @@ Size ImplToolItem::GetSize( sal_Bool bHorz, sal_Bool bCheckMaxWidth, long maxWid
> {
> if ( bHorz )
> {
> - aSize.Width() = mnSepSize;
> + aSize.Width() = mnSepSize+150;
The 150 again.
I think this patch should not go in.
Eike
--
LibreOffice Calc developer. Number formatter stricken i18n transpositionizer.
GPG key ID: 0x65632D3A - 2265 D7F3 A7B0 95CC 3918 630B 6A6C D5B7 6563 2D3A
For key transition see http://erack.de/key-transition-2013-01-10.txt.asc
Support the FSFE, care about Free Software! https://fsfe.org/support/?erack
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/libreoffice/attachments/20130517/1f8087fc/attachment.pgp>
More information about the LibreOffice
mailing list