gEDA-dev: Fix for bug #1527465: zoom extents when window is maximised

[Carlos Nieves Ónega]

Hi Patrick,
if you think I created a mess in the CVS, I apology for it. It was not
my intention to create a mess, but to improve the suite. Next time I'll
wait until you commit your patches yourself.

It's easy to revert to the previous state, since the last three commits
to gschem and libgeda are because of your 3 patches committed. 

In gschem, see:
http://cvs.seul.org/viewcvs/viewcvs.cgi/eda/geda/gaf/gschem/ChangeLog?rev=1.523&view=log
You can see that revision 1.521 through 1.523 (current) are the 3
commitments of your patches.

libgeda was only changed once (ChangeLog rev. 1.348), deleting the
changes made in 1.347 (previous fix of bug #1527465).

So I think it's easy to revert the changes, just going back to the
previuos state.

See some comments below.

El jue, 05-10-2006 a las 12:21 +0200, Patrick Bernaud escribió:
[snip]
> Here is how I think it should have been done:
> 
>   - Patch 1: alternate fix for Bug#1527465
> 
>      1 - revert the changes you made (previous fix) gschem and libgeda
>      - this one is not actually in my tree since I have not yet updated.
The changes I made to libgeda are not currently in CVS, since I deleted
them when committing your patch.

In gschem, previous changes were made to x_event.c (changes to
x_event_configure and adding new function x_event_window_state),
x_window.c (connecting the signal window_state to x_event_window_state)
and prototype.h (new definition of x_event_window_state).

The function x_event_window_state was deleted when committing your
patch, thus removing the definition from prototype.h and unconnecting
the signal in x_window.c. Result: the previous fix was deleted.

>      2 - clean up of x_event_configure() - this is quite a changes
>      and as such I will certainly not garantee it is bug-free.
The function x_event_configure which is currently in CVS is the one you
had in your patch. It seems to work correctly, and the code looks good.
There is no trace of the previous bug fix in this function.

No code in CVS or stable releases is considered bug-free. 
We have the releases so people can have some stable versions, and the
CVS is the developer version, which I think means that is not stable,
neither bug-free, and sometimes it's not working properly.


>      3 - apply new fix (possibly after discussion).

I was the person addressing this bug during the sprint code, so I felt I
knew the associated problems when dealing with it.

Your patch looked good, and worked great. 
Yes, I could have written a mail saying "your patch is great, please
commit it to CVS". As I had some time, I preferred to save you some
time.

>   This way we can track each and every changes. We have the
>   possibility to go back on any of them, easily identify when a
>   particular appeared and maintain a clean history.
> 
>   Right now if you want to go back and remove this commit you are back
>   with the previous problematic fix. Not a good solution IMHO.

If you go back to the right version, you will have the CVS state before
commiting your patches. 

Regarding this specific patch, the result is the same: the previous fix
was deleted and your fix is in CVS.

I agree that I could have reverted the previous fix, going back to the
previous version, and then aplying your patch, thus loosing that step in
the CVS history. 

However, I prefer that the ChangeLog and the CVS history shows all. It's
like: we tried it this way, but we thought the right way is this other
one. I think we have nothing to hide.

>   - Patch 2: filter as user types and clear button
> 
>      1 - add a clear button to the filter of the component selection
>      dialog
>      2 - modify component selection dialog for filter as user types

>   - Patch 3: new close dialog
> 
>      1 - add new functions to manipulate pages in windows - pretty big
>      changes in key places of the code and covers code that you
>      modified recently.
>      2 - add new dialog for user confirmation before closing page or
>      window.
> 
> Order of the patches is not significant here.
> 
> Please note how the changes are progressive and how easy it would be
> to revert one or identify a faulty one (alsmost systematically) and
> fix it.

Agree.

>  > In the last commit, I grouped all the ChangeLog entries into one, so
>  > there weren't 3 different entries with the same author and the same
>  > date. I only removed the date and author line of the other two
>  > commitments. Maybe that's the reason you were not so happy?
> 
> The date of an entry has to be the date the changes reached the
> CVS. No problem with that. However modifying an entry (the date in
> this case) at the next commit that is totally unrelated, is to say the
> least messy. 

I didn't modify the date. It was always the right one (october, 4th).
I meant that, before the commitment, the ChangeLog looked like:

2006-10-04  Patrick Bernaud  <your-email at your_provider.com>
	[change 3]

2006-10-04  Patrick Bernaud  <your-email at your_provider.com>
	[change 2]

2006-10-04  Patrick Bernaud  <your-email at your_provider.com>
	[change 1]

and afterwards it looked like:

2006-10-04  Patrick Bernaud  <your-email at your_provider.com>
	[change 3]

	[change 2]

	[change 1]

I think it's not so problematic, and it is easier to see that all
changes were made the same day by the same person.

> Plus you forgot include/x_compselect.h on patch 2.

No, you did. Look at the ChangeLog in CVS:
	* src/x_compselect.c, include/x_compselect.h: Added a button to 
	clear the filter entry of the component selection dialog.
	Modified to auto-update the component selection as user types in
	filter entry.

I didn't applied your patch directly. I was reviewing it and made some minor 
changes (like this one and adding the a_zoom_extents after calling the new 
page hook).

> I am open to discussion on this point but to me the ChangeLog must
> reflect the changes commited. It means logically grouping files in
> entries even if it implies repetitions from one line to another (not
> the case here) and avoiding changes on previous things like the
> plague.

FMPOV, the ChangeLog clearly shows the changes done:

2006-10-04  Patrick Bernaud  <your-email at your-provider.com>
	* src/x_compselect.c, include/x_compselect.h: Added a button to 
	clear the filter entry of the component selection dialog.
	Modified to auto-update the component selection as user types in
	filter entry.

	* include/prototype.h, src/x_dialog.c: Deleted old exit_dialog.
	Added new dialog for user confirmation before closing a page 
         or a window.
	* src/i_callbacks.c, src/x_fileselect.c: Adapted code to use new
	functions of x_window.c.

	* src/x_window.c: Added new functions to open, close, save and
	change page.

	* src/x_event.c (x_event_configure): Cleaned up.
	Fixed Bug#1527465: fit page zooms when window is maximized.

There are 5 points there. They are ordered from most recent (top) to
less recent (bottom).

First point was added in Changelog version 1.522.

Next three: close confirmation dialog (point 2) and new functions to
manipulate the pages (points 3 and 4) were committed together, mainly
because they were in the same patch and because the changes relating the
close confirmation dialog weren't made in the same files as the page
manipulation functions. 

I agree I could have splitted this one into two different commitments.
However, you can agree that the changes are orthogonally: they were made
in different files, so it's not so difficult to revert some of them.

--- End of comments

I'll let you decide what to do now. You have access to CVS, so you can
do whatever you want. I'm ok if you want to revert all the changes.

I tested your changes, they looked great and they were a natural
evolution of the tools, so I did what I do with other patches: if they
are good and improve the tools, they go into CVS. 
I think everyone will agree that they are great improvements.

Regarding the problems with the component selector (the drawing problems
and the escape hits), the problems were there before these changes. So I
felt there was no reason to wait for them to be fixed before aplying
these patches.

Again, thank you for your work, and my apologies for the commitments.
Next time I won't apply your patches, but wait until you do it yourself.

Regards,

Carlos