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
Attachment:
pgpv3sV0gl_eG.pgp
Description: PGP signature