[PATCH] [WIP] fdo#45747 - [EasyHack] remove the limitation to 3 sort entries in calc, part 2

Hi Markus,
So I have hacked a bit on the UI for the new sort dialog in calc. I'm
posting my work-in-progress patch hoping to get confirmation that I'm
on the right track and also help with the specific issues below.

As you suggested I've used the same concept for as in namedlg, where I
created ScSortKeyDlg class which defines the dialog for a single sort
key entry. this class is then instantiated via a control class.

So far so good I think, then comes the ugly bits:

1.) Obviously I'm using the wrong widget class for ScSortKeyDlg since
each dialog opens in a separate window and are not embedded in the
main dialog.
Which widget should I use here?

2.)  Related to the above is the question about how to position the
sort key dialogs relative to each other.

3.)  The Listbox handler. The handler is used to enable/disable the
widigets of the subsequent sort key(s). Now that each sort key is an
instance of its own I'm not sure how to get "the signal across" form
one instance to the other. Can I catch this event in the control
class?

Any hints on the above are much appreciated.

Thanks in advance.

/Albert

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Hey Albert,


> So I have hacked a bit on the UI for the new sort dialog in calc. I'm
> posting my work-in-progress patch hoping to get confirmation that I'm
> on the right track and also help with the specific issues below.


>
> As you suggested I've used the same concept for as in namedlg, where I
> created ScSortKeyDlg class which defines the dialog for a single sort
> key entry. this class is then instantiated via a control class.

It is a bit more complex in this case. We have the Dialog class that
contains the window. In the dialog class we will have some buttons and
a control that provides the list functionality. This control will then
contain all sort entries which are itself new controls. We have at
least one dialog which is already working like that:

So what we need to do right now is to first create the control for a
sort entry. That should be quite straight forward and will mostly be
copying the existing elements into an own control. Then create a
control that provides a list functionality (we already have such
controls, we just need to adapt some methods) and write the scroll
handler for this control.

Then we should be nearly ready to include it into the Dialog.

>
> So far so good I think, then comes the ugly bits:
>
> 1.) Obviously I'm using the wrong widget class for ScSortKeyDlg since
> each dialog opens in a separate window and are not embedded in the
> main dialog.
> Which widget should I use here?

Yeap. You need to derive from a Control and not from a Window. A good
example for such a concept is SfxDocumentInfoDialog with the
CustomProperties* classes. They already implement something similar
and we can copy most of the ideas from there.

>
> 2.)  Related to the above is the question about how to position the
> sort key dialogs relative to each other.

We need to write the code that calculates the position of sort
entries. We will position them inside another control so the
positioning has to be done only relative to the control.

>
> 3.)  The Listbox handler. The handler is used to enable/disable the
> widigets of the subsequent sort key(s). Now that each sort key is an
> instance of its own I'm not sure how to get "the signal across" form
> one instance to the other. Can I catch this event in the control
> class?
>

There are at least two solutions to this. First one is to give the
sort entry a reference to the class that needs to know about it or
implement the handler already in that class.

Regards,
Markus
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Servus Markus,

Just an update on what I've been doing on this.

On Sun, Apr 29, 2012 at 4:37 PM, Markus Mohrhard
<[hidden email]> wrote:
I've solved this part like this:
Tab page class <-> Controller class <-> Window class <-> Struct
{FixedLine, ListBox, RadioBtnUp, RadiobtnDown }

Basically copying the set-up of SfxDocumentInfoDialog, I've tested
this part and it works like a charm!

>>
>> 2.)  Related to the above is the question about how to position the
>> sort key dialogs relative to each other.
>
> We need to write the code that calculates the position of sort
> entries. We will position them inside another control so the
> positioning has to be done only relative to the control.

I've implemented the scroll handler but I haven't tested it yet.

This part was simple, It is basically the same handler as before, only
slightly modifed. For now i put this implementation in the Window
class, but I'm thinking of moving it back were it was before i.e in
the tabpage class. I've tested this as it is now and it works!

Overall I'm still contemplating a bit where to put the code containing
all the the logic behind the dialog. My second attempt here was to
move everything from the tab page class to the window class, but I
this turned out to create more messy code than I had anticipated.

So now I'm opting to keep much of the code intact in the tab page
class. I need to re-wind a bit here, before I have a working patch to
submit.

/Albert
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Hi Marcus,
Here is an updated patch which I hope you have time to review. I'm
sure that there are things that can be done differently, but basically
the patch works.

I have found two problems myself that I need help resolving:

1.) Compile time waring about resources ids. There is probably a
simple fix for this, I think I understand the problem but not what to
do about it.
f4099: "/home/thuswa/work/libo2/sc/source/ui/src/sortdlg.src", line
60: Warning in the object (Type: ListBox, 15):
Global resources should have an identifier >= 256.

Pos = MAP_APPFONT ( 172 , 11 ) ;
^
f4099: "/home/thuswa/work/libo2/sc/source/ui/src/sortdlg.src", line
68: Warning in the object (Type: RadioButton, 16):
Global resources should have an identifier >= 256.

Pos = MAP_APPFONT ( 172 , 25 ) ;
^
f4099: "/home/thuswa/work/libo2/sc/source/ui/src/sortdlg.src", line
75: Warning in the object (Type: RadioButton, 17):
Global resources should have an identifier >= 256.

Pos = MAP_APPFONT ( 6 , 0 ) ;
^
f4099: "/home/thuswa/work/libo2/sc/source/ui/src/sortdlg.src", line
82: Warning in the object (Type: FixedLine, 14):
Global resources should have an identifier >= 256.
[ build DEP ] SRS:sc/res


2.) to return a pointer to the vector containing the ui widgets is of
course not ideal, see below:

ScSortKeyItems* ScSortKeyWindow::AddSortKey( sal_uInt16 nItem )
ScSortKeyItems* ScSortKeyCtrl::AddSortKey( sal_uInt16 nItem )

Instead I probably should create a reference when I initialize
ScSortKeyCtrl and ScSortKeyWindow, but I'm not exactly sure how to do
this.

Thanks for your help.
/Albert

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Hello Albert,

Sorry that it took me a bit to review your patch but my master trees
are totally screwed for my own feature. I now checked your patch and
solved the conflicts that were related to my last fix for sort
dialogs. Attached you'll find a patch that applies cleanly on master.
I checked the dialog and it looks like we need to move some items a
bit.

2012/5/14 Albert Thuswaldner <[hidden email]>: This one is quite simple. we need to move the identifiers out of the 0
to 255 range.

Andras, do you have an idea what range is useable in sc for this? I
think most of the 256 to 999 range is already used but I have no idea
where to check this.
I've attached a simple patch showing how to do this. The patch will
not compile but shows already the basic ideas. The idea is to move the
ScSortKeyItems variable into tpsort and only pass a reference. I think
a good final step is to use a boost::ptr_vector which will control the
lifetime of the contained objects.

Regards,
Markus

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Hi Markus,

Forgot to CC the list in my previous mail, sorry for the noise.
The patch in the last mail was also wrong.

On Fri, May 18, 2012 at 12:10 PM, Markus Mohrhard
<[hidden email]> wrote:
> Hello Albert,
>
> Sorry that it took me a bit to review your patch but my master trees
> are totally screwed for my own feature. I now checked your patch and
> solved the conflicts that were related to my last fix for sort
> dialogs. Attached you'll find a patch that applies cleanly on master.

Thanks.

> I checked the dialog and it looks like we need to move some items a
> bit.

Yea, that's related to different themes/widget set, I guess. It looks
great on my computer running LO under KDE. I had a fixed offset which
dictated the spacing between two sort key items, in the new patch I
now calculate the spacing based on position and height of the widgets.
Still not perfect, needs some wiggling.
It would be great if someone could help out with this :)

> I've attached a simple patch showing how to do this. The patch will
> not compile but shows already the basic ideas. The idea is to move the
> ScSortKeyItems variable into tpsort and only pass a reference. I think
> a good final step is to use a boost::ptr_vector which will control the
> lifetime of the contained objects.

Thanks for the code pointer regarding the reference, It was along the
line what I was thinking. I've now implemented this using
boost::ptr_vector in the new patch. It seems to work, however
strangely enough LO now segfaults on exit of the sort dialog. The
stack trace suggest the problem to be in the Window class, however
boost::ptr_vector should take care of the deletion of its members, so
no dedicated destructor should be necessary. I'm confused...

Could you have a look at this problem. Thanks. I'm feeling the heat of
the feature freeze...

/Albert

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Hi Albert,

2012/5/18 Markus Mohrhard <[hidden email]>:
>
> This one is quite simple. we need to move the identifiers out of the 0
> to 255 range.
>
> Andras, do you have an idea what range is useable in sc for this? I
> think most of the 256 to 999 range is already used but I have no idea
> where to check this.

Sorry, I did not notice this mail. I hope it's not too late. Please
find attached a Perl script. You can move this script and scen-US.res
in a folder, run 'perl ./index.pl scen-US.res | sort -n' and you will
see the used global identifiers in sc module. However, I find peculiar
that you put a Listbox and and Fixedline outside a dialog context, on
their own. If it works, why not, it just looks strange to me. :)

Good luck!

Andras

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Hi Andras,
Thanks for taking your time to reply.

On Tue, May 22, 2012 at 5:50 PM, Andras Timar <[hidden email]> wrote: I ran your script and it worked as expected. I've updated the global
ids accordingly. Thanks for your help. :)

> However, I find peculiar
> that you put a Listbox and and Fixedline outside a dialog context, on
> their own. If it works, why not, it just looks strange to me. :)

Well I'm only copying an existing design based on Marcus advice. So it
is not my crazy idea. ;)

/Albert

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Hey Albert,

2012/5/24 Albert Thuswaldner <[hidden email]>:
I've looked at your patch and it looks really nice. Just one minor
question: Is there an add and a remove button?

I already had a quick look at the closing problem and it seems it is
not a crash. It looks more like a missing close handler which results
in the parent window being closed which is in this case the LibO
window but I need to spend some more time on it. I will spend next
week on your patch and make sure that it is ready for 3-6.

Regards,
Markus
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Hey Albert,

>
> I already had a quick look at the closing problem and it seems it is
> not a crash. It looks more like a missing close handler which results
> in the parent window being closed which is in this case the LibO
> window but I need to spend some more time on it. I will spend next
> week on your patch and make sure that it is ready for 3-6.

I finally had time to fix the issue with your patch and pushed it to master.

The problem has been that vcl's window class destructor checks that
all child windows are destroyed in dbgutil builds and your design did
not ensure this. The changes that I did to your patch are in
http://cgit.freedesktop.org/libreoffice/core/commit/?id=57e35b0ed54a2e74c107493869e72ab7eb86222a.

I also fixed a crash that I noticed when using more than 3 sort
entries. I checked and the dialog looks good now.

Thanks a lot for this amazing work. I added it to the 3.6 release notes page.

Regards,
Markus
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

HI Marcus,
Sorry for not replying until now. Forgot to CC the list again...

On Tue, Jun 5, 2012 at 3:13 AM, Markus Mohrhard
<[hidden email]> wrote:
Thanks for fixing this and getting it into 3.6! Also thanks for your
support during this work, I could not have done this without your
help. I'm very happy to see this being finished, although not so happy
about how long it took me to do this, but at least I learned a lot
along the way.

> Thanks a lot for this amazing work. I added it to the 3.6 release notes page.

I added a reference to the bug and a screenshot to the release notes.

> Regards,
> Markus

I have a few things lined up which I want to do next. For instance, do
you mind if I take a stab at this one:
https://bugs.freedesktop.org/show_bug.cgi?id=43937
I guess to fix this bug the best would be to implement the same
UI-magic here, as what I just did for the sort dialog.


/Albert
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Hey Albert,

>
>> Thanks a lot for this amazing work. I added it to the 3.6 release notes page.
>
> I added a reference to the bug and a screenshot to the release notes.

Great. The screenshot looks really good.

> I have a few things lined up which I want to do next. For instance, do
> you mind if I take a stab at this one:
> https://bugs.freedesktop.org/show_bug.cgi?id=43937
> I guess to fix this bug the best would be to implement the same
> UI-magic here, as what I just did for the sort dialog.
>

Oh this one is already implemented just not yet pushed. If you're
interested in this area there are many more dialogs that would benefit
from a rework. Please let me know if this is something you want to do.
Then we could discuss with the UX guys which dialog they would prefer
to convert.

Regards,
Markus
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice