RFC: GThesaurus.C et al.

[Bernhard Reiter]

being one of those apparently sometimes popping-by gtk afficionado lyx
users and trying to get myself in touch with the gtk part of the lyx
sources, i spent sunday evening putting together the thesaurus dialog
for gtk. this is my first contact with both the lyx sources and gtk
programming (and in fact my first [wannabe] contribution to an open
source project), so please spare me if my work isn't flawless... the
thesaurus dialog seemed kinda ok for the beginning as it wasn't horribly
complex and hasn't yet been checked off on your guii page...

screenshot: Loading Image...

i had some trouble using cvs diff (another first-time experience) -- i
don't know how to deal with newly generated local files that aren't in
cvs yet, so there are three of them in the attached archive (.C and .h
go to src/frontends/gtk/, .glade to src/frontends/gtk/glade/) plus one
patch adding them to Dialogs.C, Makefile.am and Changelog. i hope i
didn't miss anything...

there is some change in the ui i'd like to point out compared to the
xforms and qt equivalent: i chose a cancel/apply/ok kind of dialog over
the replace/close approach (with the buttons on top of each other which
i really found a little strange); i also removed the lower 'Selection'
textbox (left to the replace button) as it only seemed to be helpful
with xforms where the current item in the listbox is not highlighted
(with gtk, it seemed just confusing). i hope there wasn't any
sophisticated detail about that approach that i overlooked... in case
you're missing the word "replace", i'd suggest changing the apply button
caption, but still leaving it in the lower row...
out of a first try, synonym lists are also altered while typing. this
works without any latency on my machine (1.4 ghz), but if that's causing
anybody's worries, changing the signal handler shouldn't be too
difficult...
i hope this implementation makes the dialog a bit more intuitive,
consistent and comfortable to use (e.g.: if no synonym in the list is
selected, e.g., but there is some text in the upper textbox ('entry' in
gtk jargon), 'apply' can be clicked and will use that text)

working on this, i'm afraid i stumbled over a bug in the thesaurus
controller (but didn't do enough research to be able to cure it):
1. highlight some word in the main window
2. open the thesaurus dialog, choose a synonym and click replace
3. choose another synonym, click replace again - it won't work

i checked this with the qt frontend. same thing, so it's apparently in
the controller's replace function... (didn't look any further into that,
admittedly...)

i like lyx (i've been using it 'in the field' last semester for
live-typing a math lecture for my physics studies and parts of a
theoretical physics one, see http://www.unet.univie.ac.at/~a0057324/ --
german only, sorry), but xforms really suck, so i'd love to see a gtk
version of lyx: is gtk support planned for 1.4.0? i could probably
convince some of my mates to use it for more collective forms of
lecture-typing if it had an interface blending in with a decent DE (no
flamewar intended:)... i'd also contribute some more code if i find some
time...

and, one more question: is there a way to enable compilation for all
three frontends? i used the qt ui/sources as a template, but seeing it
in live action would have been quite useful in parallel.

i also have to say that i'm not really aware of what bc().valid(bool)
(as opposed to set_sensitive(false)), applylock = false etc do. (some
hints/pointers?) thus, the 'readonly'/valid thing is probably the
ugliest aspect of my code (taking care of all the cases when greying out
the apply button is appropriate and when not...)

regards
bernie
sorry, that was lengthy...

[Angus Leeming]

Post by Bernhard Reiter
being one of those apparently sometimes popping-by gtk afficionado lyx
users and trying to get myself in touch with the gtk part of the lyx
sources, i spent sunday evening putting together the thesaurus dialog
for gtk. this is my first contact with both the lyx sources and gtk
programming (and in fact my first [wannabe] contribution to an open
source project), so please spare me if my work isn't flawless... the
thesaurus dialog seemed kinda ok for the beginning as it wasn't horribly
complex and hasn't yet been checked off on your guii page...

Hi, Bernhard, and welcome!

Post by Bernhard Reiter
i had some trouble using cvs diff (another first-time experience) -- i
don't know how to deal with newly generated local files that aren't in
cvs yet, so there are three of them in the attached archive (.C and .h
go to src/frontends/gtk/, .glade to src/frontends/gtk/glade/) plus one
patch adding them to Dialogs.C, Makefile.am and Changelog. i hope i
didn't miss anything...

You didn't. In order to use 'cvs add' you have to have write access to the
repository. What you've posted is just fine.

[snip detailed discussion of UI difference...]

Post by Bernhard Reiter
i hope this implementation makes the dialog a bit more intuitive,
consistent and comfortable to use (e.g.: if no synonym in the list is
selected, e.g., but there is some text in the upper textbox ('entry' in
gtk jargon), 'apply' can be clicked and will use that text)

The change in UI is fine. That's one of the points of having multiple
frontends; no one person can have all the best ideas.

Post by Bernhard Reiter
working on this, i'm afraid i stumbled over a bug in the thesaurus
1. highlight some word in the main window
2. open the thesaurus dialog, choose a synonym and click replace
3. choose another synonym, click replace again - it won't work
i checked this with the qt frontend. same thing, so it's apparently in
the controller's replace function... (didn't look any further into that,
admittedly...)

If you do look in the controller (I did for the first time just now!)
you'll see that oldstr_, the string you're replacing, is set when the
dialog is opened, in initialiseParams.

Call replace(newstr). oldstr_ is replaced throughout your document.
Call replace(newstr2). There's no oldstr_ left to replace.

I'd suppose that oldstr_ should be replaced by newstr at the end of the
first call to replace(newstr), but I'll leave it to you to think about
this further.

Post by Bernhard Reiter
is gtk support planned for 1.4.0?

No. The frontend is clearly a work-in-progress.

Post by Bernhard Reiter
and, one more question: is there a way to enable compilation for all
three frontends? i used the qt ui/sources as a template, but seeing it
in live action would have been quite useful in parallel.

Here I use:
configure --with-version-suffix \
--disable-stdlib-debug \
--with-frontend='qt xforms gtk' \
--with-qt-includes='/usr/include/qt3'

Post by Bernhard Reiter
i also have to say that i'm not really aware of what bc().valid(bool)
(as opposed to set_sensitive(false)), applylock = false etc do. (some
hints/pointers?) thus, the 'readonly'/valid thing is probably the
ugliest aspect of my code (taking care of all the cases when greying out
the apply button is appropriate and when not...)

OK.

One thing before I move on to the code. Can I have your formal agreement
that you agree to licence your contributions under the terms of the GNU
General Public Licence (GPL) version 2 or later? It's enough to reply to
this mail. I'll add a reference to your "yes" to
http://www.lyx.org/about/blanket-permission.php

On to the code...

I've never looked in GTK's Dialogs.C before. First thing I see when I do so
is that the code initially sets the ButtonController to be the XForms one
and then overwrites this in the creation of many of the individual
dialogs. I think that I'd move

dialog->bc().view(new xformsBC(dialog->bc()));

to those dialogs that actually need it...

By the way, your GThesaurus.patch has a CVS conflict in the ChangeLog.
That's those <<<<<<< .... ========== ... >>>>>>>> bits. CVS is unsure
what's going on here so adds the placeholders as pointers for you to
examine.

GThesaurus.h:
/** This class provides a GTK+ implementation of the FormSearch Dialog.
I don't think it does...

You might add some more doxygen comments (start /// or /**) to describe
what individual functions do. Most are obvious I guess, but I'd need to
look at the implementation to guess what the three non-virtual functions
do.

GThesaurus.C:
no real comments.
A stylistic one: use tab-indentation please.
What happens if you remove the applylock_ and readonly lines entirely? (I'm
unfamiliar with them although I do know the button controller code.)

I don't believe that you need 'bc().refreshReadOnly();'. In face, I'd
remove applylock, readonly and bc().valid(false) entirely from doBuild.
Their settings should be set, if at all, in update().

Have a look in controllers/Dialogs.C to get a handle on the overall logic
behind the Controller-View split. Without this overview, you're fighting
with one arm behind your back and one eye closed.

Well done!

[Bernhard Reiter]

first things first...

Post by Angus Leeming
One thing before I move on to the code. Can I have your formal agreement
that you agree to licence your contributions under the terms of the GNU
General Public Licence (GPL) version 2 or later? It's enough to reply to
this mail. I'll add a reference to your "yes" to
http://www.lyx.org/about/blanket-permission.php

Sure:
I grant permission to license any and all contributions I've made to
LyX under the Gnu GPL version 2 or later.

Post by Angus Leeming

Post by Bernhard Reiter
working on this, i'm afraid i stumbled over a bug in the thesaurus
1. highlight some word in the main window
2. open the thesaurus dialog, choose a synonym and click replace
3. choose another synonym, click replace again - it won't work
i checked this with the qt frontend. same thing, so it's apparently in
the controller's replace function... (didn't look any further into that,
admittedly...)

If you do look in the controller (I did for the first time just now!)
you'll see that oldstr_, the string you're replacing, is set when the
dialog is opened, in initialiseParams.
Call replace(newstr). oldstr_ is replaced throughout your document.
Call replace(newstr2). There's no oldstr_ left to replace.
I'd suppose that oldstr_ should be replaced by newstr at the end of the
first call to replace(newstr), but I'll leave it to you to think about
this further.

as far as i can see, only the current occurence of oldstr_ is
replaced...
assigning oldstr_ = newstr afterwards solves the problem if there is
only one occurence of oldstr_, but is not sufficient if there are more
than one: in (anon)::replace (lyxfind.C) -- which is indirectly called
by LFUN_WORD_REPLACE -- ::find is called, so the next occurence of
oldstr_ is selected automatically. there seems to have been a parameter
'bool once' in 1.3.6's LyxFind, but it looks like it has vanished
since...
as far as i can see, changing back the thesaurus's behavior would
require such a parameter in the replace LFUN -- and i'm not sure if i'm
in the position to solely decide about interface changes of that kind...
other options i can think of are leaving the thesaurus's behavior this
spellchecker-like way (but i doubt this is desirable) or calling the
find LFUN with fw=false (which i'd consider a waste of time and
resources). i don't know if there are any ready-to-use LFUNs (instead of
LFUN_WORD_REPLACE, that is), though.

Post by Angus Leeming

Post by Bernhard Reiter
is gtk support planned for 1.4.0?

No. The frontend is clearly a work-in-progress.

Post by Bernhard Reiter
and, one more question: is there a way to enable compilation for all
three frontends? i used the qt ui/sources as a template, but seeing it
in live action would have been quite useful in parallel.

configure --with-version-suffix \
--disable-stdlib-debug \
--with-frontend='qt xforms gtk' \
--with-qt-includes='/usr/include/qt3'

thanks, that helped a lot.

Post by Angus Leeming
On to the code...
I've never looked in GTK's Dialogs.C before. First thing I see when I do so
is that the code initially sets the ButtonController to be the XForms one
and then overwrites this in the creation of many of the individual
dialogs. I think that I'd move
dialog->bc().view(new xformsBC(dialog->bc()));
to those dialogs that actually need it...

done that for now.

Post by Angus Leeming
By the way, your GThesaurus.patch has a CVS conflict in the ChangeLog.
That's those <<<<<<< .... ========== ... >>>>>>>> bits. CVS is unsure
what's going on here so adds the placeholders as pointers for you to
examine.

oops, seems i messed around with cvs update and diff a little too
much... i'll take better care in the future, i promise :)

Post by Angus Leeming
/** This class provides a GTK+ implementation of the FormSearch Dialog.
I don't think it does...

oops again. couldn't that damn clipboard or gedit be more intelligent
(than me)?

Post by Angus Leeming
You might add some more doxygen comments (start /// or /**) to describe
what individual functions do. Most are obvious I guess, but I'd need to
look at the implementation to guess what the three non-virtual functions
do.

ok, i added some, hopefully sufficient.

Post by Angus Leeming
no real comments.
A stylistic one: use tab-indentation please.
What happens if you remove the applylock_ and readonly lines entirely? (I'm
unfamiliar with them although I do know the button controller code.)
I don't believe that you need 'bc().refreshReadOnly();'. In face, I'd
remove applylock, readonly and bc().valid(false) entirely from doBuild.
Their settings should be set, if at all, in update().
Have a look in controllers/Dialogs.C to get a handle on the overall logic
behind the Controller-View split. Without this overview, you're fighting
with one arm behind your back and one eye closed.
Well done!

thanks for the warm welcome. i hope i'll gradually require less
attention, but at the moment it certainly helps a lot getting started...
i hope i covered all your and John's suggestions and the whole thing
uses the right controller methods now. applylock_ remains there -- i
couldn't make up anything better to prevent the dialog from inserting
text if none was selected when it was opened...

bernie

[Angus Leeming]

Post by Bernhard Reiter
first things first...

Post by Angus Leeming
One thing before I move on to the code. Can I have your formal agreement
that you agree to licence your contributions under the terms of the GNU
General Public Licence (GPL) version 2 or later? It's enough to reply to
this mail. I'll add a reference to your "yes" to
http://www.lyx.org/about/blanket-permission.php

I grant permission to license any and all contributions I've made to
LyX under the Gnu GPL version 2 or later.

Thank you.

[Georg Baum]

Post by Angus Leeming

Post by Bernhard Reiter
I grant permission to license any and all contributions I've made to
LyX under the Gnu GPL version 2 or later.

Thank you.

Angus, do you plan to commit Bernhards work? If not, I will just do it,
since the gtk frontend is not in freeze, and this is certainly an
improvement.


Georg

[Angus Leeming]

Post by Georg Baum
Angus, do you plan to commit Bernhards work? If not, I will just do
it, since the gtk frontend is not in freeze, and this is certainly
an improvement.

Go for it.

On a more general level, at what point should be scrub the "XForms
exception" from the licence and from the web pages?

[Georg Baum]

Post by Angus Leeming

Post by Georg Baum
Angus, do you plan to commit Bernhards work? If not, I will just do
it, since the gtk frontend is not in freeze, and this is certainly
an improvement.

Go for it.

Done (minus the controller change, see patch).

Post by Angus Leeming
On a more general level, at what point should be scrub the "XForms
exception" from the licence and from the web pages?

IMHO: When all people who contributed significantly have agreed to the pure
GPL.


Georg

[Angus Leeming]

Post by Georg Baum
IMHO: When all people who contributed significantly have agreed to
the pure GPL.

Right. Well here's the list of the only people who I either never
traced or who never replied to my requests.

I'm sure that one of the old timers could get a response out of Mate,
Claus and Miyata. The rest I'm not too worried about.

Christian Buescher (User-definable keys, lyxserver...)
Francesc Burrull i Mestres (Catalan translation)
Claudio Coco (Italian translation)
John Michael Floyd (Bug fix to the spellchecker)
Claus Hentschel (Win32 port of LyX 1.1.x)
Carmen Kauffmann (Original name)
KDE Artists (Authors of several of the icons)
Peter Kremer (Hungarian translation)
Victor Lavrenko (Russian translation)
Tino Meinen (Dutch translation)
Dirk Niggemann (config. handling enhancements,
bugfixes...)
Joacim Persson (po-file for Swedish, bugfixing...)
Geoffroy Piroux (Mathematica backend for mathed)
Bernhard Psaier (Designer of the LyX-Banner)
Hubert Schreier (spellchecker)
Ivan Schreter (slovak, czech, german stuff)
Miyata Shigeru (OS/2 port)
David Suárez de Lis (Spanish translation)
Mate Wierdl (@lists.lyx.org)
Serge Winitzki (Scientific Word bindings)
Xiaokun Zhu (bug reports and small fixes)

[John Levon]

Post by Angus Leeming
KDE Artists (Authors of several of the icons)

Isn't KDE already GPL'd ? Therefore by merely shipping the icons with
our software, we never changed their licenses anyway.

regards
john

[John Spray]

Oooh! Oooh! gtk frontend!

Couple points, haven't looked at the code in detail:
* The .glade file needs an entry in
frontends/gtk/glade/Makefile.am to get installed.
* The .glade file should have the separator disabled on the dialog
object to be consistent with the others and GNOME standards, the
property's called "Hat Trennlinie" if you're running in de.

John