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



_______________________________________________
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Reply via email to