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

[Sander van Grieken]

On Friday 28 May 2010 05:59:58 Joshua Judson Rosen wrote:
> Sander van Grieken <sander at 3v8.net> writes:
> > > 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? :)

Ok ok I have to choose my words carefully :) So it's a change, but not a redesign :)

No zen koan yet, but I suspect we might invent one along the way with these discussions :)

> > 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.
> 
> > 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.

Yes :) And I see the advantages of that.

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

Since I have been through all the code the last few weeks, it was sometimes just too 
tempting to *not* go outside the branch scope. But yeah, I'll try to restrain myself in 
the future, or branch mercilessly if I really cannot hold back :)

> 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.'

Ok I can revert these.

>     Stylistic changes(?):
> 
>         * `move parse_degrees to support.[ch]'
> 
>         * `move string (un)escape functions to support.[ch]'

Maybe these can stay in? They haven't changed internally.

>         * 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").

These two are for consistency among poi_t, photo_t and friend_t. It also avoids having to 
use convertor macros everywhere. I think it's also in scope for the branch, since osm-gps-
map takes degrees in many cases, where tango used radians (although not consistently).

>         * 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....

Yes you're right, these have to go.

> ... 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.

It's not meant as hand-waving. I know 'diff' can ignore most of the whitespace changes, 
and it would be nice if bzr can merge this way. After merging, my branch dies anyway, and 
any new branch will be (re)based on trunk.

> > > 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).

It's only worse if you merge blindly, which you already said you didn't do. Besides, any 
merge has the potential to break things, I think I did pretty well so far, you found the 
only regression I introduced (fingers crossed :) )

> 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. :)

In this case I think it was unavoidable since integrating osm-gps-map touched that part of 
the code anyway, regardless of the out-of-scope changes.

> > 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.

Maybe this is easier and/or faster:
- cherrypick the out-of-scope changes that you want in trunk from my branch (like the 
seen_vaild typo, the move of support functions to support.[ch], xml parsing crash fix)
- I rebase my branch on trunk again, so those changes 'disappear'
- I revert the changes you *don't* want in trunk
- finally, merge

> 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

I'm surprised. For someone that's quite strict about the process to be loose about this.

> > > 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 :)

Well there has been more out-of-scope stuff going into my branch than I initially thought, 
so I won't burden you with that. Please consider my 'reverse' cherry-picking proposal 
above.

> > 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.... :)

hahaha ok ok. Let's find a solution we can all be happy with.

Thanks for the thorough response.

grtz,
Sander