Re: new xforms patch

[Marko Vendelin]

On Sat, 14 Oct 2000, Allan Rae wrote:

 On 13 Oct 2000, Jean-Marc Lasgouttes wrote:
 
   "Marko" == Marko Vendelin [EMAIL PROTECTED] writes:
  
   Marko is there any particular reason for using NULL instead of 0 in
   a lot of your code? Is this a gnome convention? Where is it
   defined?
  
  Marko I am just used to call NULL a pointer that might lead to a core
  Marko dump. I haven't looked into gnome conventions recently, but if
  Marko you will give me a good reason to use integer 0 insted of NULL
  Marko then I can change everywhere NULL to this particular integer
  Marko value.
  
  I could not find a reference to it anywhere, but I am sure Lars will
  be able to remind us why NULL is evil :)
 
 I'll supply one:  NULL is a C'ism while 0 is C++.
 
 That's why I asked where NULL is defined.  I suspect you are getting NULL
 from a C header file where it is probably defined as (void*)0 or something
 similar -- if you write 0 the C++ compiler will consider it to be
 static_castWhateverType *(0) and the 0 will therefore have an
 _appropriate_ type.
 
 If you were being tricky and setting all your dynamic memory to 0x0a or
 some other marker to help detect uninitialised memory I could understand
 you using something other than 0 to initialise a pointer (provided the
 rest of your code was full of Assert()'s checking the pointers against
 that value).  In C++ if you mean zero you write 0.  IIRC, this is
 mentioned in both the C++ FAQ and Effective C++.

I see. I'll submit a patch as soon as I'll be back in my office (in a week
or so).

Marko

Re: new xforms patch

[Marko Vendelin]

On Sat, 14 Oct 2000, Allan Rae wrote:

> On 13 Oct 2000, Jean-Marc Lasgouttes wrote:
> 
> > > "Marko" == Marko Vendelin <[EMAIL PROTECTED]> writes:
> > 
> > >> Marko is there any particular reason for using NULL instead of 0 in
> > >> a lot of your code? Is this a gnome convention? Where is it
> > >> defined?
> > 
> > Marko> I am just used to call NULL a pointer that might lead to a core
> > Marko> dump. I haven't looked into gnome conventions recently, but if
> > Marko> you will give me a good reason to use integer 0 insted of NULL
> > Marko> then I can change everywhere NULL to this particular integer
> > Marko> value.
> > 
> > I could not find a reference to it anywhere, but I am sure Lars will
> > be able to remind us why NULL is evil :)
> 
> I'll supply one:  NULL is a C'ism while 0 is C++.
> 
> That's why I asked where NULL is defined.  I suspect you are getting NULL
> from a C header file where it is probably defined as (void*)0 or something
> similar -- if you write 0 the C++ compiler will consider it to be
> static_cast(0) and the 0 will therefore have an
> _appropriate_ type.
> 
> If you were being tricky and setting all your dynamic memory to 0x0a or
> some other marker to help detect uninitialised memory I could understand
> you using something other than 0 to initialise a pointer (provided the
> rest of your code was full of Assert()'s checking the pointers against
> that value).  In C++ if you mean zero you write 0.  IIRC, this is
> mentioned in both the C++ FAQ and Effective C++.

I see. I'll submit a patch as soon as I'll be back in my office (in a week
or so).

Marko

Re: new xforms patch

[Marko Vendelin]

 Overall the xforms stuff is looking very nice.  I took at peek at the
 gnome and kde stuff as a result of the change to update and
 updateBufferDependent.  There seems to be an aweful lot of code just to
 get things running with gnome.  

True! There are two reasons for it. First, I decided to code some dialogs
and then compose the base classes looking into the implementation of
several dialogs. This approach helped me to learn LyX and its requirements
to compose base classes better. Second, all the dialogs which use "action"
area have to be constructed on "fly" and change/update on the basis of
user input. This makes using the dialogs easier, faster and much more
fun, but coding ... well, you mentioned a size of the code :). 

 There's also a few connections being made to update that aren't used
 (since update is empty) is this a temporary thing? (ie. do you plan on
 filling in some more code?)

Most (even it seems to me that all) of these empty update functions are in
"action" area dialogs. My latest patch connects updateBufferDependent to
hide function. The reasons for it are as follows.

If I understand correctly "update" is called when user opens a new
document or changes the document. If this is true then for "action" area
dialogs the update signal has to close the dialog since user is not
interested in this action anymore :). At least it is easier to implement
than keeping action area state for each document... 

 There also appears to be a few places where a new update_something()
 function exists that could be used as update() and an update connection
 would then make sense.

the quick grep for update in gnome sources showed only updateButtons in
FormCitation that might look as a candidate for an update connection and
which doesn't use it. However, updateButtons is used to track user input
in FormCitation dialog and has nothing to do with change of the document.


 Marko is there any particular reason for using NULL instead of 0 in a lot
 of your code? Is this a gnome convention? Where is it defined?

I am just used to call NULL a pointer that might lead to a core dump. I
haven't looked into gnome conventions recently, but if you will give me a
good reason to use integer 0 insted of NULL then I can change
everywhere NULL to this particular integer value.


 BTW, I've said it before but I'll say it again anyway since it's friday:
 You don't have to call your files by the same name as the xforms code.
 I would have expected KDE for example to use DialogX myself -- or
 whatever naming convention exists for apps using a particular toolkit.

Unfortunately, the gnome frontend is still lagging behind xforms. That's
why it is easier to use the same names for the dialogs since it gives a
good overview of the state of the port. Finally, it is nice to remember
roots of LyX when everyone will switch to Gnomified version :).

[...]

 There that's probably pissed nearly all the GUII devvies off.

No way! It is very nice to have someone who is looking into the code. It
is a first step to make this effort in porting LyX to gnome or kde from
one-man project to dynamic project full of
discussions/suggestions/implementations as the rest of LyX is. Actually,  
this is a reason why I would not like to have cvs-write access anytime
soon. This guarantees that the code was working at least in two occasions
and someone has at least just scanned the patch.


 Have a nice Friday 13th,

Enjoy it too.

Marko

Re: new xforms patch

[Jean-Marc Lasgouttes]

 "Marko" == Marko Vendelin [EMAIL PROTECTED] writes:

 Marko is there any particular reason for using NULL instead of 0 in
 a lot of your code? Is this a gnome convention? Where is it
 defined?

Marko I am just used to call NULL a pointer that might lead to a core
Marko dump. I haven't looked into gnome conventions recently, but if
Marko you will give me a good reason to use integer 0 insted of NULL
Marko then I can change everywhere NULL to this particular integer
Marko value.


I could not find a reference to it anywhere, but I am sure Lars will
be able to remind us why NULL is evil :)

JMarc

Re: new xforms patch

[Allan Rae]

On 13 Oct 2000, Jean-Marc Lasgouttes wrote:

  "Marko" == Marko Vendelin [EMAIL PROTECTED] writes:
 
  Marko is there any particular reason for using NULL instead of 0 in
  a lot of your code? Is this a gnome convention? Where is it
  defined?
 
 Marko I am just used to call NULL a pointer that might lead to a core
 Marko dump. I haven't looked into gnome conventions recently, but if
 Marko you will give me a good reason to use integer 0 insted of NULL
 Marko then I can change everywhere NULL to this particular integer
 Marko value.
 
 I could not find a reference to it anywhere, but I am sure Lars will
 be able to remind us why NULL is evil :)

I'll supply one:  NULL is a C'ism while 0 is C++.

That's why I asked where NULL is defined.  I suspect you are getting NULL
from a C header file where it is probably defined as (void*)0 or something
similar -- if you write 0 the C++ compiler will consider it to be
static_castWhateverType *(0) and the 0 will therefore have an
_appropriate_ type.

If you were being tricky and setting all your dynamic memory to 0x0a or
some other marker to help detect uninitialised memory I could understand
you using something other than 0 to initialise a pointer (provided the
rest of your code was full of Assert()'s checking the pointers against
that value).  In C++ if you mean zero you write 0.  IIRC, this is
mentioned in both the C++ FAQ and Effective C++.

Allan. (ARRae)

Re: new xforms patch

[Marko Vendelin]

> Overall the xforms stuff is looking very nice.  I took at peek at the
> gnome and kde stuff as a result of the change to update and
> updateBufferDependent.  There seems to be an aweful lot of code just to
> get things running with gnome.  

True! There are two reasons for it. First, I decided to code some dialogs
and then compose the base classes looking into the implementation of
several dialogs. This approach helped me to learn LyX and its requirements
to compose base classes better. Second, all the dialogs which use "action"
area have to be constructed on "fly" and change/update on the basis of
user input. This makes using the dialogs easier, faster and much more
fun, but coding ... well, you mentioned a size of the code :). 

> There's also a few connections being made to update that aren't used
> (since update is empty) is this a temporary thing? (ie. do you plan on
> filling in some more code?)

Most (even it seems to me that all) of these empty update functions are in
"action" area dialogs. My latest patch connects updateBufferDependent to
hide function. The reasons for it are as follows.

If I understand correctly "update" is called when user opens a new
document or changes the document. If this is true then for "action" area
dialogs the update signal has to close the dialog since user is not
interested in this action anymore :). At least it is easier to implement
than keeping action area state for each document... 

> There also appears to be a few places where a new update_something()
> function exists that could be used as update() and an update connection
> would then make sense.

the quick grep for update in gnome sources showed only updateButtons in
FormCitation that might look as a candidate for an update connection and
which doesn't use it. However, updateButtons is used to track user input
in FormCitation dialog and has nothing to do with change of the document.


> Marko is there any particular reason for using NULL instead of 0 in a lot
> of your code? Is this a gnome convention? Where is it defined?

I am just used to call NULL a pointer that might lead to a core dump. I
haven't looked into gnome conventions recently, but if you will give me a
good reason to use integer 0 insted of NULL then I can change
everywhere NULL to this particular integer value.


> BTW, I've said it before but I'll say it again anyway since it's friday:
> You don't have to call your files by the same name as the xforms code.
> I would have expected KDE for example to use DialogX myself -- or
> whatever naming convention exists for apps using a particular toolkit.

Unfortunately, the gnome frontend is still lagging behind xforms. That's
why it is easier to use the same names for the dialogs since it gives a
good overview of the state of the port. Finally, it is nice to remember
roots of LyX when everyone will switch to Gnomified version :).

[...]

> There that's probably pissed nearly all the GUII devvies off.

No way! It is very nice to have someone who is looking into the code. It
is a first step to make this effort in porting LyX to gnome or kde from
one-man project to dynamic project full of
discussions/suggestions/implementations as the rest of LyX is. Actually,  
this is a reason why I would not like to have cvs-write access anytime
soon. This guarantees that the code was working at least in two occasions
and someone has at least just scanned the patch.


> Have a nice Friday 13th,

Enjoy it too.

Marko

Re: new xforms patch

[Jean-Marc Lasgouttes]

> "Marko" == Marko Vendelin <[EMAIL PROTECTED]> writes:

>> Marko is there any particular reason for using NULL instead of 0 in
>> a lot of your code? Is this a gnome convention? Where is it
>> defined?

Marko> I am just used to call NULL a pointer that might lead to a core
Marko> dump. I haven't looked into gnome conventions recently, but if
Marko> you will give me a good reason to use integer 0 insted of NULL
Marko> then I can change everywhere NULL to this particular integer
Marko> value.


I could not find a reference to it anywhere, but I am sure Lars will
be able to remind us why NULL is evil :)

JMarc

Re: new xforms patch

[Allan Rae]

On 13 Oct 2000, Jean-Marc Lasgouttes wrote:

> > "Marko" == Marko Vendelin <[EMAIL PROTECTED]> writes:
> 
> >> Marko is there any particular reason for using NULL instead of 0 in
> >> a lot of your code? Is this a gnome convention? Where is it
> >> defined?
> 
> Marko> I am just used to call NULL a pointer that might lead to a core
> Marko> dump. I haven't looked into gnome conventions recently, but if
> Marko> you will give me a good reason to use integer 0 insted of NULL
> Marko> then I can change everywhere NULL to this particular integer
> Marko> value.
> 
> I could not find a reference to it anywhere, but I am sure Lars will
> be able to remind us why NULL is evil :)

I'll supply one:  NULL is a C'ism while 0 is C++.

That's why I asked where NULL is defined.  I suspect you are getting NULL
from a C header file where it is probably defined as (void*)0 or something
similar -- if you write 0 the C++ compiler will consider it to be
static_cast(0) and the 0 will therefore have an
_appropriate_ type.

If you were being tricky and setting all your dynamic memory to 0x0a or
some other marker to help detect uninitialised memory I could understand
you using something other than 0 to initialise a pointer (provided the
rest of your code was full of Assert()'s checking the pointers against
that value).  In C++ if you mean zero you write 0.  IIRC, this is
mentioned in both the C++ FAQ and Effective C++.

Allan. (ARRae)

Re: new xforms patch

[Allan Rae]

On Thu, 12 Oct 2000, Allan Rae wrote:
 Good news... I'll apply it to my tree. 
 
 and then I'll do the stuff below:
 
  An alternative fix would be by making
  Signal1void, bool updateBufferDependent;
  
  Such that true == "buffer change", and false == "same buffer".
 
 [...]
 
 There are a few things that'll be easy to clean up before I commit it.
 (mostly related to redefining the updateBufferDepedent signal)

I've run out of time and have to go out tonight so you'll just have to
wait till tomorrow.

Allan. (ARRae)

Re: new xforms patch

[Angus Leeming]

 [sigh] Didn't I tell you not to run off and implement this stuff for a few
 days so we could have time to think about it. ;-)

;-) Things as they were were just t nasty! I blame you for pointing 
out just how nasty!

 An alternative fix would be by making
   Signal1void, bool updateBufferDependent;

 What do you think of this?
Magnificent idea! Clean, simple and elegant.

 Why is it then that you wanted to add an ihSignal_ in FormInset when it
 would make more sense to just make the connection in the appropriate
 showInset() like it used to be done?  Over zealous perhaps?

Well, it struck me that all those insets with dialogs will emit a hide signal 
when deleted or cut, so the right place to connect this signal to the dialog 
itself would be in FormInset::connect(). Unfortunately, the base Inset class 
does not have a hide signal (ie, no Inset::hide), so I had to obtain 
InsetXXX::hide in the various showInset() methods (stored as ihSignal_) and 
then connect it in FormInset::connect().

I know... kludgy! However, I think that the idea is basically correct. The 
right place to connect ih_ IS in Form::Inset::connect(). All that needs to be 
done is to make an Inset::hide signal rather than have all those 
InsetXXX::hide signals.

My turn to ask: what do you think of this?

Angus

Re: new xforms patch

[Allan Rae]

On Thu, 12 Oct 2000, Allan Rae wrote:
> Good news... I'll apply it to my tree. 
> 
> and then I'll do the stuff below:
> 
> > An alternative fix would be by making
> > Signal1 updateBufferDependent;
> > 
> > Such that true == "buffer change", and false == "same buffer".
> 
> [...]
> 
> There are a few things that'll be easy to clean up before I commit it.
> (mostly related to redefining the updateBufferDepedent signal)

I've run out of time and have to go out tonight so you'll just have to
wait till tomorrow.

Allan. (ARRae)

Re: new xforms patch

[Angus Leeming]

> [sigh] Didn't I tell you not to run off and implement this stuff for a few
> days so we could have time to think about it. ;-)

;-) Things as they were were just t nasty! I blame you for pointing 
out just how nasty!

> An alternative fix would be by making
>   Signal1 updateBufferDependent;

> What do you think of this?
Magnificent idea! Clean, simple and elegant.

> Why is it then that you wanted to add an ihSignal_ in FormInset when it
> would make more sense to just make the connection in the appropriate
> showInset() like it used to be done?  Over zealous perhaps?

Well, it struck me that all those insets with dialogs will emit a hide signal 
when deleted or cut, so the right place to connect this signal to the dialog 
itself would be in FormInset::connect(). Unfortunately, the base Inset class 
does not have a hide signal (ie, no ::hide), so I had to obtain 
::hide in the various showInset() methods (stored as ihSignal_) and 
then connect it in FormInset::connect().

I know... kludgy! However, I think that the idea is basically correct. The 
right place to connect ih_ IS in Form::Inset::connect(). All that needs to be 
done is to make an Inset::hide signal rather than have all those 
InsetXXX::hide signals.

My turn to ask: what do you think of this?

Angus

Re: new xforms patch

[Allan Rae]

On Wed, 11 Oct 2000, Angus Leeming wrote:

 Attached is a patch implementing Allan's suggestions about a FormInset base 
 class. I've actually implemented three small new classes:
 
 FormBaseBI and FormBaseBD are base classes for Buffer Independent and Buffer 
 Dependent dialogs respectively. FormInset is, in turn, derived from 
 FormBaseBD.

[sigh] Didn't I tell you not to run off and implement this stuff for a few
days so we could have time to think about it. ;-)

It occured to me that we could make things clearer by just merging the two
enums in FormBase to:
enum BufferDependency {
BUFFER_UPDATE,  // always update
BUFFER_CHECK,   // same buffer then update else hide
INDEPENDENT
}

This removes the confusion I mentioned in FormPreferences, although it
still leaves the buffer check problem.

An alternative fix would be by making
Signal1void, bool updateBufferDependent;

Such that true == "buffer change", and false == "same buffer". This is
simple enough and we could then eliminate the keeping of a Buffer*
anywhere.  We wouldn't need the updateOrHide() stuff at all.  We wouldn't
need the modified BufferDependency enum above or the ChangedBufferAction
enum.  The various update() members would need to change to accept a bool
but they could/should also default to false.  The testing of whether to
hide or update can then be done in the update() where it belongs.  (It'd
also mean we wouldn't need most of your patch -- see why I told you not to
run off and implement the FormInset stuff: some other idea came up)

What do you think of this?

I'll take a look at your patch and see what I think of that.
Frustrating aren't I?

 I've also got my head around overloading connect() and disconnect() in
 the daughter classes.  Incidentally, Allan, I think that the names of
 these methods are fine. Think of them as meaning "things to be done on
 connection" rather than simply "connect signals".

Fair enough.  I'll add a longer comment to that effect to FormBase.
 
 All this gives no new functionality, just cleans up the code base. Hope that 
 its now logical.

We probably still want a FormInset anyway.  Although if we adopt my
suggestion for changing the updateBufferDependent signal much of what you
have written will need to be rewritten (or just culled).  I'm inclinded to
just leave the BufferDependency stuff in FormBase rather than add another
layer in the form of FormBaseB[DI].

 One new piece of functioality has been added, however. I've used some of 
 Baruch's code from FormGraphics in the FormInset-derived classes. If a dialog 
 is open and recieves a new "Show" or "Create" signal from another inset, then 
 the dialog is updated with the new inset's data. No need to first close the 
 dialog to one inset before opening it for another.

Yay!

Allan. (ARRae)

Re: new xforms patch

[Allan Rae]

On Thu, 12 Oct 2000, Allan Rae wrote:

 On Wed, 11 Oct 2000, Angus Leeming wrote:
 
  Attached is a patch implementing Allan's suggestions about a FormInset base 
  class. I've actually implemented three small new classes:
  
  FormBaseBI and FormBaseBD are base classes for Buffer Independent and Buffer 
  Dependent dialogs respectively. FormInset is, in turn, derived from 
  FormBaseBD.
 
 [sigh] Didn't I tell you not to run off and implement this stuff for a few
 days so we could have time to think about it. ;-)
[...]

Good news... I'll apply it to my tree. 

and then I'll do the stuff below:

 An alternative fix would be by making
   Signal1void, bool updateBufferDependent;
 
 Such that true == "buffer change", and false == "same buffer".

[...]

  I've also got my head around overloading connect() and disconnect() in
  the daughter classes.  Incidentally, Allan, I think that the names of
  these methods are fine. Think of them as meaning "things to be done on
  connection" rather than simply "connect signals".

Why is it then that you wanted to add an ihSignal_ in FormInset when it
would make more sense to just make the connection in the appropriate
showInset() like it used to be done?  Over zealous perhaps?

There are a few things that'll be easy to clean up before I commit it.
(mostly related to redefining the updateBufferDepedent signal)

Allan. (ARRae)

Re: new xforms patch

[Allan Rae]

On Wed, 11 Oct 2000, Angus Leeming wrote:

> Attached is a patch implementing Allan's suggestions about a FormInset base 
> class. I've actually implemented three small new classes:
> 
> FormBaseBI and FormBaseBD are base classes for Buffer Independent and Buffer 
> Dependent dialogs respectively. FormInset is, in turn, derived from 
> FormBaseBD.

[sigh] Didn't I tell you not to run off and implement this stuff for a few
days so we could have time to think about it. ;-)

It occured to me that we could make things clearer by just merging the two
enums in FormBase to:
enum BufferDependency {
BUFFER_UPDATE,  // always update
BUFFER_CHECK,   // same buffer then update else hide
INDEPENDENT
}

This removes the confusion I mentioned in FormPreferences, although it
still leaves the buffer check problem.

An alternative fix would be by making
Signal1 updateBufferDependent;

Such that true == "buffer change", and false == "same buffer". This is
simple enough and we could then eliminate the keeping of a Buffer*
anywhere.  We wouldn't need the updateOrHide() stuff at all.  We wouldn't
need the modified BufferDependency enum above or the ChangedBufferAction
enum.  The various update() members would need to change to accept a bool
but they could/should also default to false.  The testing of whether to
hide or update can then be done in the update() where it belongs.  (It'd
also mean we wouldn't need most of your patch -- see why I told you not to
run off and implement the FormInset stuff: some other idea came up)

What do you think of this?

I'll take a look at your patch and see what I think of that.
Frustrating aren't I?

> I've also got my head around overloading connect() and disconnect() in
> the daughter classes.  Incidentally, Allan, I think that the names of
> these methods are fine. Think of them as meaning "things to be done on
> connection" rather than simply "connect signals".

Fair enough.  I'll add a longer comment to that effect to FormBase.
 
> All this gives no new functionality, just cleans up the code base. Hope that 
> its now logical.

We probably still want a FormInset anyway.  Although if we adopt my
suggestion for changing the updateBufferDependent signal much of what you
have written will need to be rewritten (or just culled).  I'm inclinded to
just leave the BufferDependency stuff in FormBase rather than add another
layer in the form of FormBaseB[DI].

> One new piece of functioality has been added, however. I've used some of 
> Baruch's code from FormGraphics in the FormInset-derived classes. If a dialog 
> is open and recieves a new "Show" or "Create" signal from another inset, then 
> the dialog is updated with the new inset's data. No need to first close the 
> dialog to one inset before opening it for another.

Yay!

Allan. (ARRae)

Re: new xforms patch

[Allan Rae]

On Thu, 12 Oct 2000, Allan Rae wrote:

> On Wed, 11 Oct 2000, Angus Leeming wrote:
> 
> > Attached is a patch implementing Allan's suggestions about a FormInset base 
> > class. I've actually implemented three small new classes:
> > 
> > FormBaseBI and FormBaseBD are base classes for Buffer Independent and Buffer 
> > Dependent dialogs respectively. FormInset is, in turn, derived from 
> > FormBaseBD.
> 
> [sigh] Didn't I tell you not to run off and implement this stuff for a few
> days so we could have time to think about it. ;-)
[...]

Good news... I'll apply it to my tree. 

and then I'll do the stuff below:

> An alternative fix would be by making
>   Signal1 updateBufferDependent;
> 
> Such that true == "buffer change", and false == "same buffer".

[...]

> > I've also got my head around overloading connect() and disconnect() in
> > the daughter classes.  Incidentally, Allan, I think that the names of
> > these methods are fine. Think of them as meaning "things to be done on
> > connection" rather than simply "connect signals".

Why is it then that you wanted to add an ihSignal_ in FormInset when it
would make more sense to just make the connection in the appropriate
showInset() like it used to be done?  Over zealous perhaps?

There are a few things that'll be easy to clean up before I commit it.
(mostly related to redefining the updateBufferDepedent signal)

Allan. (ARRae)