Re: [linux-minidisc] Implementing downloading in QHiMDTransfer

[Michael Karcher <Michael.Karcher@fu-berlin.de>]

On Sun, 2012-11-04 at 14:52 +0100, Kevin Chabowski wrote:
> I now implemented downloading MP3 tracks in the GUI.
Thanks for your work on our project! I will review your patches in this
mail.

> For this, I moved some code from himdcli to libhimd, so there are not that  
> much duplicated code.
Great stuff. That's exactly the approach we need.

> It did not work for every MP3 file I tried, but these also failed using  
> the current himdcli version.
In my oppinion, the MP3 download code is still beta quality, so things
like that are expected. It would be interesting to know whether
re-uploading the non-working MP3 files to the computer yields working
files. Also it would be interesting to get a dump of the codec-specific
info of the non-working files. I don't think any tool is currently
printing it, but you should be able to either hack himdcli into it, or
get it from hexdumps of the TRKIDX0?.HMA file.

> Also after formatting the HiMD using the  
> player (MZ-RH10) I was not able to download any MP3.
Try recording another track (for example using the MZ-RH10) first. This
sounds like a bug in the trackindex write function.

> Again, this also  
> happened using the himdcli version from the current master branch. Is this  
> a known issue?
While MP3 writing has been in himdcli for some time to push the MP3
writing code, that code has neither received big amounts of constructive
criticism or refactoring, nor big amounts of testing. It's mostly of
"works for me for more than one file" quality. So I don't know the
issues you cite, but I am not seriously confused by problem reports on
the writing code.

| From fe57e862779da63061d9bd939201dbf574a445d3 Mon Sep 17 00:00:00 2001
| From: Kevin Chabowski <kevin@kch42.de>
| Date: Sat, 13 Oct 2012 23:14:39 +0200
| Subject: [PATCH 01/16] Replaced on_localScan_clicked with selectionChanged handler
That patch looks fine.

| From 7d4d3f6f373efae13a8b022dbc90a17f5b5ed01d Mon Sep 17 00:00:00 2001
| From: Kevin Chabowski <kevin@kch42.de>
| Date: Sat, 13 Oct 2012 23:19:57 +0200
| Subject: [PATCH 02/16] Replaced Uploaddialog with Up/Downloaddialog
I would suggest avoiding the clunky term "updownload" in method names,
even if the method was named upload previously. So rename
"upload_canceled" to just "cancelled" (fixing a spelling error by the
way). One could also think about using the term "transfer" instead of
"updownload".
The change to qhimdmainwindow.ui included in that patch seems not to be
related to that patch, but possibly due to different font sizes or DPI
values of your computers. Please remove it.
I dislike the repeated pattern of 'is_upload_dialog ? tr("...") :
tr("...")' scattered all over your code.
I would suggest to add QString members to the class and initialize them
in the constructor like

| if(is_upload_dialog) {
|   error_string = tr("%1 track(s) could not be uploaded");
|   success_string = tr("%1 track(s) successfully uploaded");
|   ...
| } else {
|   error_string = tr("%1 track(s) could not be downloaded");
|   success_string = tr("%1 track(s) successfully downloaded");
|   ...
| }

Another way would be to generate to constant string tables like

| struct messages {
|   const char *error_string;
|   const char *success_string;
|   ...
| };
| 
| const struct messages upload_messages = {
|   "%1 track(s) could not be uploaded",
|   ...
| };
| 
| const struct messages download_messages = {
|   "%1 track(s) could not be downloaded",
|   ...
| };

and then include a messages* member to your dialog. The structure could
be local to the QHiMDUpDownloadDialog class, and the two tables could be
constant static members.
And finally there is the weasel way of just talking about "transferring"
instead of "uploading" and "downloading".
I don't see any QHiMDUploadDialog2 or QHiMDDownloadDialog templates
defined in your .ui files. Either you forgot to add the relevant .ui
files, or the forward declarations in qhimdupdownloaddialog.h are
irrelevant.
By the way, you might want to try the "--find-renames" parameter to git
format-patch (or any other diffing operation).


| From 3f835ddb210a95570b0769b4e45f2dcbe68edaea Mon Sep 17 00:00:00 2001
| From: Kevin Chabowski <kevin@kch42.de>
| Date: Mon, 15 Oct 2012 21:28:08 +0200
| Subject: [PATCH 03/16] Activating download button, if appropiate.
As you noted yourself, this patch is irrelevant due to merging the MP3
filter patch by Thomas Arp.

| From aaa81f44e5b2fd183e251b905d95d5dee9425022 Mon Sep 17 00:00:00 2001
| From: Kevin Chabowski <kevin@kch42.de>
| Date: Tue, 16 Oct 2012 17:29:22 +0200
| Subject: [PATCH 04/16] Removed file extension check from local selection  change.
The himd_devices->count logic in this patch seems reasonable at the
first sight, though I did not verify what it means if
himd_devices->count is bigger than 1 (probably multiple connected
devices) and whether you target the right one. OTOH, maybe you just want
to check whether the HiMD is opened?

As a side note: patch series are unable to represent merges, so bindly
applying your series will fail since the merge is not in. The better
practice to deal with concurrent changes that get into a project while
you are working yourself on it would be to "rebase" your stuff on the
new version, instead of merging. You might want to get to know the
powers of git-rebase (especially the interactive mode, for example "git
rebase -i HEAD~4" to be able to "modify history" in your last 4 patches.

| From 2183f98221644a4380b6f2346a0c312099d80495 Mon Sep 17 00:00:00 2001
| From: Kevin Chabowski <kevin@kch42.de>
| Date: Tue, 16 Oct 2012 17:31:47 +0200
| Subject: [PATCH 05/16] Moved get_songinfo from himdcli to libhimd
This patch violates a design idea of libhimd. As I did not document that
idea anywhere, it's not your fault, though. The idea was that while
libhimd (currently) uses glib internally, the use of glib is not exposed
on the interface. The needless use of gchar instead of char in the
output pointer parameters (I know, you just copied/moved it) is one of
the reasons for pulling in glib.h in a public header of libhimd.
Furthermore a gchar** might trap the reader into the idea that he/she
should use g_free to free the allocated value. But in fact, using plain
free would be right.

The other reason of pulling glib.h in the public interface is the
gboolean return value. Please also adopt to the scheme used in HiMD that
negative return values mean failure and zero means success, you might
consider adopting the convention of a himderrinfo parameter, too.

I hope removing <glib.h> from himd.h also removes the need to add
glib-2.0 to the PKG_CONFIG line of qhimdtransfer.pro. Also note that
libhimd.pro already contains a PKG_CONFIG line that includes glib-2.0,
so you might add id3tag to that line instead of adding another line and
duplicating glib-2.0 on it. Finally, if I remember correctly, the
create_prl option creates a .prl file that already includes the
libraries needed to build libhimd, so no need to add id3tag explicitly
to the qhimdtransfer.pro file, too.

An ugly thing in your patch (sticking out in review, not in the code) is
the space change in the himd_writemp3 call. You changed a tab character
into 8 spaces (or your editor did). As you did not intend to change
anything on that line, please do not change the whitespace thing in the
same commit as you use for significant modification, to ease
interpreting "git blame" output. I have no problems accepting a patch
that replaces all tab characters in himdcli.c by 8 spaces (they came
into that file when we merged the MP3 writing code), as long as that
patch does not do anything else and is clearly described as "cleanup"
patch in its commit message.

Finally, every function exported from libhimd starts with "himd_".
Please keep it that way, or invent a prefix for MP3 related functions
like "mp3_". Don't export unprefixed functions, because on unix, symbol
resolution (static linking as well as run-time library loading) does not
tie symbols to library names.

Please squash this patch with the next one (make one patch that contains
the result of applying both in order).

| From e096e749587e50da5c72bb0afa7da5bbd0e1c725 Mon Sep 17 00:00:00 2001
| From: Kevin Chabowski <kevin@kch42.de>
| Date: Mon, 22 Oct 2012 21:43:35 +0200
| Subject: [PATCH 07/16] himd_set_track_label implemented
This patch definitely does not pass quality control. While you describe
being able to change existing labels, and it appears to work fine at the
beginning, you are leaking HiMD strings. The 14-byte string fragments
that are unused are organized in a linked freelist. When you change a
title, you have to add all the string fragments used by the title back
into the freelist (and change the type nibble to zero) to prevent
loosing these "string slots" until the next formatting of the disk. 

While usually, a blatantly reject an error return out of a function that
takes a himderrinfo* parameter if the errinfo is not set, a silent
"return FALSE" in case of a bad label_type parameter is acceptable. In
that case, the caller violated the precondition of providing a valid
label_type value (which by the way could be declared as enum instead of
int, and the three constants can be the enumerator values). But please
do parameter validation using g_return{_val}_if_fail, because that
function prints a nice error message in debug builds if the precondition
is violated.

The himd_modify_track approach is quite dangerous, as it relies on the
user to pass the right track index to it. I think I would prefer a
function himd_start_update_track_info that initializes a structure of
type this type:
struct himd_editable_trackinfo {
	struct trackinfo data;
	int index;
};
and a function himd_commit_update_track_info that takes the index from
that structure, which has been initialized by
himd_start_update_track_info. For bonus points, set index to -1 in the
start function if it fails, and blatantly refuse to commit updates with
negative index.
The remark about not using glib types (like gboolean) in the public
interface of libhimd also applies to himd_set_track_label.

| From ffd91b653bef249d38d9ecde18a5d993fc8a081d Mon Sep 17 00:00:00 2001
| From: Kevin Chabowski <kevin@kch42.de>
| Date: Mon, 22 Oct 2012 22:15:00 +0200
| Subject: [PATCH 08/16] Using himd_set_track_label in himd_writemp3 of himdcli
This patch looks fine, except for error handling (printing a warning) if
himd_set_track_label fails.

Further reviews in a later mail, it's late enough, and I already spent
an hour reviewing up to here. The time it takes is fine, a good review
takes that long, but I have to get some sleep, too. I hope to be able to
send you a review of the next 8 patches soon.

Finally, thanks again for spending your time on improving QHiMDTransfer!

Best regards,
  Michael Karcher