Re: [Audacity-devel] Help system patches for review.

[Gale <ga...@au...>]

Richard Ash (audacity-help) wrote
> On Thu, 21 Aug 2014 00:14:48 +0100
> Steve the Fiddle <

> stevethefiddle@

> > wrote:
> 
>> Hi Richard,
>> 
>> I've now had time to look at the patches that you posted.
>> Thanks for taking the time to do that, and sorry that it wasn't
>> clearer in my original patch.
>> 
>> Your extended comments improve clarity. The /// and @param are for
>> doxygen?
> Yes - although because I use it all the time at work, I am now used to
> reading the plain text comments as much as the Doxygen output. I'm
> trying to use it to record anything which I have to break off and
> figure out, in the hope of not having to do so in the future.
> 
>> I think patches 001 to 004 are pretty self explanatory now.
> Yes - these are simple enough once they are all separated from each
> other.
> 
>> helpsystem-005.patch
>> Yes, I was wanting to add an optional argument to control the modality
>> of the wxHTML window.
>> My first attempt was more like how you have it now, but that's when I
>> got the "ambiguous" problem.
>> I'm unsure what I did wrong the first time, but your version is what I
>> originally wanted to do.
> I think the way the diff presented this (which isn't as you explained
> it, because diff doesn't read minds) made this look nastier than it
> was. You also added your modal argument to the other overload with the
> default the other way, which confused me!
> 
>> So the bit that you really want me to explain is
>> helpsystem-remainder.patch
> Yes - I ran out of re-work time.
>  
>> "HelpServerRoot" and "HelpAlphaRoot" are where the "home page" is
>> located. In the case of the alpha manual this is currently the same
>> as the rest of the pages, but as that might change in the future I
>> thought best to put it in now,
>> 
>> "Main Page" and "Quick Help" need to be handled differently from the
>> rest of the manual because for the release and locally installed
>> manuals they  are outside of the /man/ folder. For some (historical?)
>> reason they also have a "pagename" that is unrelated to the name of
>> the file.
> I thought this was the intention, but got confused in the detail (and
> pushed for time). I think this needs a longer explanatory comment
> somewhere, which notes which constants are used in each of case. If my
> choice of constant names is then confusing, feel free to change them
> (for purposes of review, please separate out the rename as a distinct
> patch from the functionality, because it's easier to review a simple
> name change, even when it touches quite a lot of places).
> 
> I think (at the moment) that it would be more readable to treat this as
> two cases (local and "online") in the main code, and then change the
> constant definitions conditionally on #if IS_ALPHA (where there are
> again only two options). This should be easier than trying to mix all
> three in the 3-way pattern you have at the moment, even if it means
> slightly more constants (some of which are identical in some
> configurations).
> 
> Main Page and Quick Help will have to be special cases, but presumably
> can be shared between offline and release, because release is just the
> offline zip placed on a server?

Release is the contents of help/manual/ placed in the  
http://manual.audacityteam.org/o/ folder

so as to give the shortest URL possible.


Gale



Richard Ash (audacity-help) wrote
> So I think this change needs to be done slightly differently for
> safety, but agree it needs doing.
> 
>> "releasePageName.Replace(wxT(":"), wxT("_"), true);" appears to be a
>> translation that was missed, but is required for transforming
>> FAQ:Installation_and_Plug-Ins#ffdown
>> into
>> faq_installation_and_plug_ins.html#ffdown
> That makes sense, but obviously happens somewhere else in the mw2html
> script than the block of transformations where the rest come from. This
> wants a patch on it's own, putting the new transformation separate from
> the rest (before or after) with a comment noting that it isn't in the
> same place in the python, but is definitely needed. Probably only two
> line change, but doing it this way makes sure it stays changed.
> 
> Can you submit this is a patch on top of the ones I posted, and
> hopefully someone will review and commit them all.
> 
> Richard





--
View this message in context: http://audacity.238276.n2.nabble.com/Help-system-patches-for-review-tp7562915p7562952.html
Sent from the audacity-devel mailing list archive at Nabble.com.