On Sunday 07 September 2008, Phil Dibowitz wrote:
> Did you see my comments on the most recent version of your concordance
> patches? All but one of them were minor - and the other was more of a
> question.
Yes, I apologize for being lazy with my mails recently - besides holidays and
and business work, I was busy shifting my system to a new disk, and then
upgrading my LINUX with new Kernel, KDE 4.1 and new soundcard.
So, for your comments from last week:
----------------
> do you think you can do me a favor and flesh out the perl driver to use
> these functions? (test.pl) That would save me time.
I'll have a look, though my knowledge of Perl is quite limited..
well, can't be much worse than Python-
> Why aren't you using a callback in learn_from_remote() (which would, I
> assume pass through to LearnIR)?
I atually considered a callback for a short while initially, but then
decided against it, since it would not give much useful information.
The most important progress for learning is the timeout for reception
of the signal (4 sec), and that is completely handled by the low-level
driver, so no way for a callback to indicate that the time is running
out - thus I made up my own 4sec gauge in the Python GUI as a separate
task. The actual reception of a IR signal then is so fast that it's over
before you've could have noticed that there was a progress bar filling up.
> You sorta fake it up in concordance.c, but LearnIR() has a lot of knowledge
> of progress, it seems and a better progress meter could be given to the
> user.
?? All concordance gives is a crude ASCII-rendering of the received signal -
after all of it is received.
> > -#include <string.h>
> > +#include <string>
> >
> >> fixed VC++ warning about deprecated include
>
> I can't reproduce it now, but I seem to remember this doing weird things in
> linux for old c-string functions. Have you tested this on Linux?
It's working fine this way with my LINUX as well (at least the parts of the
code that I tested), but I can't speak for older versions.
> <some formatting issues>
On my list for this week - hopefully I got the indentation settings of
kdevelop right now.. Is there something like Java 'checkstyle' for C/C++?
> I noticed you use a do-while instead of a while-do. Is there a reason? If
> they're the same upon entrance to the function, should you still run?
If what's the same? *keyname != "KeyName" only makes sense after calling
GetTag at least once, in order to proceed to the next tag. The initial
value of *keyname is irrelevant.
> Do-while should only be used in that scenario (despite it's regular use in
> our code).
I don't see anything wrong here - the code says:
- do { get next token
- return error if there isn't any more
- } while ( desired token not found )
> concordance/concordance.1
...
> I see no need to specify that learn-ir supports multiple commands. It
> should have always, and most users are only going to be confused by this.
No idea why this should confuse anyone. IMHO it's worth to be mentioned
that you can navigate through the list BOTH forward and back (which AFAIR
you can't in Logitech's web interface).
> Please explain (in a comment) that your set_canon() function changes the
> behavior of getchar() - this is not at all clear.
Done.
> +#define USE_DEFAULT '\n'
> This name is horrible, how about, NEWLINE or SYSTEM_NEWLINE?
If you say so...
> Also, windows doesn't give '\r', it gives '\r\n' where as unix gives just
> \n. So your stuff works kinda by accident, but that should probably be
> fixed.
I'll check that.
>
> + /*
> + * full dump of new IR signal:
> + */
>
> Typo?
Nah, just some unemployed TABs that were looking for a job :)
> Does it make sense to maybe put this prompt string in a variable and use
> that instead? I think it would make the code much more readable.
So would (IMHO) setting shiftwidth=3, tabstop=3, expandtab and textwidth=100..
Seriously, I would rather keep prompt, key choices and defaut choice strings
in place. I don't see any such string variables anwhere else in the code
anyway...
> + err = _learn_ir_commands(data,
> size, &options);> This doesn't need to start with an underscore. None of the
> mode functions do.
Probably left over from establishing this as a local wrapper for the original
function imported from libconcord, and then extended to the other new
functions:
> please do rename _printburst and _printspace
> and _dump_new_code to something like print_ir_burst, print_ir_space, and
> dump_ir_code.
Adding that to my list.
> Also, I notice you do index++ at the beginning, and then have all these
> cases where it you index-- or index -= 2.
> Is there any reason not to change to this approach?
Not really - fixed that in my code right now.
Just a few more days, and an updated patch should be ready.
Andreas
-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
concordance-devel mailing list
concordance-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/concordance-devel
