[FOSS-GPS] Re: Integrating osm-gps-map into FoxtrotGPS

[Joshua Judson Rosen]

Sander van Grieken <sander at 3v8.net> writes:
> On Tuesday 25 May 2010 15:49:16 Joshua Judson Rosen wrote:
> > Sander van Grieken <sander at 3v8.net> writes:
> > >
> > > Everything is now integrated (so also the friends, POI and photo
> > > icons), quite a number of pre-existing bugs are fixed too, and the
> > > few things that are not yet possible with the current osm-gps-map
> > > (like line-drawing in distance mode) are annotated with TODOs in the
> > > code. The code has changed much, so any work based on trunk now will
> > > be difficult to merge later.
> > 
> > You actually have several orthogonal things going on in that branch,
> > which makes it difficult to merge: the biggest one is that you've
> > integrated osm-gps-map, but you've also done some redesign of the GUI,
> 
> No I haven't actually changed the GUI. I have replaced one toolbutton with a 
> tooltogglebutton though..

Is this a zen koan? When is a change not a change? :)

> > renamed variables, renamed callbacks, moved functions to different
> > files, 
> 
> Yes. Since the statements within those callbacks totally changed
> anyway I went ahead and gave them more intuitive names, but AFAIR
> *only* on the callbacks that were impacted anyway.

What about the variables and struct-members? :)

> Maybe that makes the changes a bit harder to follow as an observer,

Indeed, it makes it harder for me to review the changes, which means
that it's more difficult to merge them :\

I just can't bring myself to merge a branch that I can't review;
of course, yours isn't anywhere near *impossible* to review, but
it may take a little longer than either of us would like.

On the up side, however: before merging this into the trunk, I'd like
to finish up the more mundane & low-risk work (like converting the GUI
to GladeXML--which is *almost* done), and then get an initial *release*
out that includes the improvements accumulated so far since we diverged
from tangoGPS (support for different gpsd versions via  libgps,
zoom-decoupling, updated translations, misc. bug-fixes, etc.) so that
others can have something that's straightforward to package and distribute...,
and I'm expecting to give that all about another week right now--hopefully
not much more than that. So, if anyone has a regression to report, or
another set of translation-updates to contribute, now would be a good time
to do that :)

The release-notes still need to be written, also; and, since I've been
unfortunately lax about creating ChangeLog entries to go along with
the changes themselves..., a ChangeLog needs to be produced.
I'm debating just adding a Makefile rule that creates a ChangeLog
from the `bzr log' output....

> but it's more easily understandable by future contributors.

Oh I didn't mean to outright reject the *content* of any of your
changes; orthogonal issues just need to be handled separately--
you'll find that I'm a stickler for `one thing at a time',
one issue/feature per branch/merge.

Of course, that's not to say that I *never* screw it up myself....

> > and made other stylistic changes to unrelated segments of code.
> 
> Yes and 99% of it is empty lines. There was lots of that. Maybe
> Marcus uses a portrait flatpanel? :)

I have no idea. There are some weird things in there, indeed--like the
"vaild_fix" typo that's somehow absolutely consistent everywhere,
all over the place. That's one fix I'd like to incorporate :)

But it's really the other `1%' is that's more confounding:

    Unrelated behavioural changes:

        * `gconf store/restore show_pois and selected POI category'

        * `don't reset input fields for routing! much nicer this way'

        * `fix autocenter treshold and make autocenter a gconf setting'

        * `make toolbar autocenter button a toggle button.'


    Stylistic changes(?):

        * `move parse_degrees to support.[ch]'

        * `move string (un)escape functions to support.[ch]'

        * Changing the datatype returned by parse_degrees().

        * Renaming the variables and struct-members used to store angles
          (dropping the "_deg" suffix from "lat_deg" and "lon_deg").

        * Removing hard linebreaks in some places to make lines extremely
          long (this I *am* actually going to just outright reject...),
          adding hard linebreaks in other places. It's not immediately
          obvious whether you actually made any functional changes
          along with this....


... and probably some more, but a full review is going to take more time
that I have available tonight--and this list is getting to the point where
I might as well just do the cherry-picks. ;)

> Haven't actually tested it, but it should be possible to ignore the
> whitespace in the diffs.

Eh. I'm not a big fan of "just ignore that" or other hand-waving
exercises; if something is just noise, and serves no purpose other
than distraction, I'd rather just not include it in the first place.

Also, even if it's ignorable in diffs (and only some of your changes are),
it's still significant in merges:

> > It looks like there was also an unnecessary merge-conflict created and
> > resolved unfavourably in at least main.c (there is a regression in the
> > "--gui=" command-line option, if I merge your branch).
> 
> Oh? My branch was rebased to trunk right before I sent out the merge
> request last saturday, and it should have been conflict free
> then.

I didn't mean that I experienced a conflict when trying to merge your
branch into the trunk, but rather that it looks like you must have
experienced a conflict when merging the trunk down into your branch--
and then you must have opted to keep the wrong half of the conflict.
I have no idea how you would have got one half of the option-parser
changes but not the other half, otherwise.

So, you're right that you're not causing any merge-conflicts on my end;
it's perhaps worse, though: I can just merge your branch into mine, and
have it `cleanly' apply the conflict-resolution choices that you made
(which break some things).

This would be where the `informatic environmentalism' pays off, of course:
if you limit the scope of your changes so as to minimise the likelihood of
creating the conflicts in the first place, the `wrong resolution'
issue doesn't even come up. :)

> But yeah, the --gui option regressed, and is fixed now.

Thanks; it's not obvious whether anything else is similarly broken,
though--and it's going to take some review to basically determine it
the hard way, since I can't necessarily just read all of the individual
diffs and then compare them piecemeal to the whole. And, even where I can
do that, there's enough `noise' in the diffs that it's going to slow me
down a bit--putting issues of `cleanliness for its own sake' aside, it
might even be *quicker* to just cherry-pick all of the desirable bits
into a new branch.

I'll probably do that, if you don't get to it first. Just let me know
whether you want your name attached to the resulting commits (bzr provides
an `Author' field in commits that can be used via, "bzr commit --author",
to indicate the author of a change if it's someone other than the committer).
Really, if I can understand what's what, it should be fairly straightforward
to do cherrypicking with something like "bzr merge --pull --change=...".

Actually, I'm wondering now whether your expecation was that I'd just
do that anyway--since you're mainly experienced with other revision-control
systems?

> > Can you prepare a branch that has just the changes necessary to integrate
> > osm-gps-map, without superfluous changes like the ones listed above
> 
> Nope. It means I have to spend a whole day to revert changes that
> are rejected on an esthetics basis and will later probably change
> anyway.

I guess I'm on the hook for that, then :)

Going that route, I'll of course publish my cherry-pick branch
*alongside* the trunk so that everyone can see what's coming.

> You're right that I should have been more conservative in
> what to change,

I guess we'll do better in the future.

> but being strict about this is mostly only useful in projects with
> many contributors that work on the same files.

Well, I'd hate for that to become a self-fulfilling prophecy.... :)

-- 
"Don't be afraid to ask (λf.((λx.xx) (λr.f(rr))))."