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
