Re: [Libreoffice] [GSOC][PATCH] Multiline inputbar

On 05/07/11 03:52, Anurag Jain wrote:
> As of now there are might be some printf's lying around so ignore them
> as of now.
> Awaiting your feedback on this.
Ok, here goes
1) ScInputBarGroup::ScInputBarGroup()

In the initialization list,
 bIsMultiLine    ( this )  is wrong surely, bIsMultiLine is of sal_Bool 
type, and there is some weird behaviour associated with that when you 
first use the input box.
2) ScInputBarGroup::ScInputBarGroup()
also
        aButton         ( this, ScResId( 1 )),

I doubt this is correct, not sure what ScResId( 1 ) corresponds to but 
surely it initialises either text or some graphic associated with the 
button, in your case you have neither, best to not pass that unless we 
specifically define or reuse some resource for the button

3) ScInputBarGroup::Resize()

    if(bIsMultiLine)
    aSize.Height()=3*TBX_WINDOW_HEIGHT;
    else
    aSize.Height()=TBX_WINDOW_HEIGHT;
can you indent that properly please

4)   ScInputBarGroup::Resize()


you set the height of the ScInputBarGroup to a the default toolbar 
window height or a multiple of the toolbar window height depending on 
the mode. It makes more sense to set the size to the size of the height 
the textbox will be for the mode ( the textbox height in single line 
mode is determined by the TBX_WINDOW_HEIGHT or the minimum edit height ) 
by exclusively using TBX_WINDOW_HEIGHT you are ignoring the fact the 
preferred height of the window could be larger than TBX_WINDOW_HEIGHT. 
But... we discussed the previously

5) ScInputBarGroup::Resize()

I don't like magic numbers, e.g. the '10' in  aSize.Width() = Max( 
((long)(nWidth - nLeft - 10)), (long)0 );
it does nothing for the readability and certainly doesn't make it easier 
to understand. I suppose it is to make to leave a little space between 
the rightmost edge of the toolbar and the InputBarGroup, if so a #define 
or const value with some some sensible name would be in order. Since the 
windows is effectively invisible not sure if leaving that space really 
gains anything. Again this was something we talked about before

6) ScInputBarGroup::Resize()

same thing with     
aButton.SetPosSizePixel(Point(aSize.Width()-40,0),Size(15,22));
make some sensible constant for 40 because that same number must be used 
where you resize the actually textwindow the button is to sit next to 
right ? also isn't is 40 a little large ?

7) ScInputBarGroup::Resize()

the button dimensions, again these are hardcoded, iiwy I would make the 
height of the button the same as the non-multiline height of the textbox 
, I would make the width some fraction of the height ( that gives it a 
pleasant aspect ).

8) ScInputBarGroup::Resize()
I see some use of Invalidate here ( and elsewhere ), I have to say I am 
not sure what the real purpose of Invalidate is, presumably it ensures 
that a window is marked dirty for the purposes of repainting or resizing 
or whatnot ( at least I have seen previously calling Invalidate 
eventually calls Resize ) But... if there is a preferred way of using it 
e.g. call SetSizePixel on a window then call Invalidate etc. I don't 
know, maybe someone else might


about Resizing in general, to me the way you seem to be daisy-chaining 
the resizing seems to make sense ( not sure from a vcl point of view if 
it does ) but at lest it is easy to follow, Toolbar:Resize calls 
InputBar::Resize which calls TextBoxThing::Resize. If it is wrong then 
at least a logical arrangement should be easy to reorganise
9)IMPL_LINK( ScInputBarGroup, ClickHdl, PushButton*, pBtn )

the check for bIsMultiline to set it e.g.

if(!bIsMultiLine)
{
    bIsMultiLine=true;
    aTextWindow.SetMultiLineStatus(true);
    Resize();
}
else
{
    bIsMultiLine=false;
    aTextWindow.SetMultiLineStatus(false);
    Resize();
}

is a little long winded, just assign bIsMultiLine  to it's logical NOT 
value and then call the following lines
   aTextWindow.SetMultiLineStatus(bIsMultiLine);
   Resize();

no need to duplicate the above lines
10) ScMultibar::ScMultibar

the order of initialisation of  bInputMode & bIsMultiLine is incorrect ( 
see warnings when you compile ) there are some other you introduced too, 
please fix them up too if they still exist after merging you code with 
the updated feature-branch

11 ) ScMultibar::Resize

hmm setting the size of the text box to the toolbox height ( or multiple 
) again here just seems wrong.  Personally I would determine the height 
to be a the normal single line mode height that is normally determined 
in the constructor of this class, in multiple-line mode I would make 
that number to be a multiple of that height.

so InputBarGroup::Resize would probably determine it's own height 
dependant on the preferred height of the textbox/ScMultiBar ( the naming 
is getting confusing here since I renamed this to ScMultiTextWnd ) - I 
will use ScMultiTextWnd in future

12) ScMultibar::Resize
shouldn't the size of the output area also be changed, even though the 
size of the ScMultiTextWnd has changed the output area is still set a 
single line.

13 ) ScMultibar::SetSize()

I don't see the point in this method, it more or less duplicates what's 
already done in Resize

14 ) ScMultibar::GetLineCount()
doesn't look correct, that would just get the current line at index 0 ( 
which will always be 1 )

After building this patch I noticed a couple of runtime funnies, it 
seems that resizing the application window even in single line mode 
doesn't adjust the display of the line contents. For example if you fill 
the ScMultiTextWnd with text till it exceeds the line and then type some 
more characters then you can use the up and down arrows to navigate 
between the lines. However if you reduce the window size you would 
expect that the number of lines for the same about of content to increase.
Similarly if you fill the ScMultiTextWnd with text till it exceeds the 
line end and then increase the size of the application window you would 
expect the 2 lines to fold back to displaying to 1 as the ScMultiTextWnd 
becomes big enough to display the entire contents in a single line ( 
small issue but one to beaware of )

also when swapping between the multi/single line mode the position of 
the text box within the window seems to just

Also I quickly played with changing the size of the toolbar itself, hmm 
I see now what you were trying to explain yesterday  it doesn't change 
size quite so readily, seems you need to enter the ScMultiTextWnd with 
the cursor click on a cell and repeat to get the size change to 
propagate. Not sure what is happening there :-/


So, if you manage to merge any changes post the patch and depending on 
time it may be possible to include extra work in master.

Noel