Re: [linux-minidisc] Implementing downloading in QHiMDTransfer

["Kevin Chabowski" <chabowsk@informatik.uni-luebeck.de>]

Sorry, that I didn't reply earlier. 3rd semester at university is much
more time consuming than I thought...

But here are finally fixes for the first 8 (now 5) patches.

Am 06.11.2012, 00:51 Uhr, schrieb Michael Karcher
<Michael.Karcher@fu-berlin.de>:
> | 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

Now I feel bad that you invested so much time to analyze that patch, since
I reverted that one some patches later, should have written so in the mail,
sorry :-/

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

Yes, that seems to be a better idea. So instead of:
| if(localmodel.fileInfo(index).isFile())
|     download_possible = (ui->himd_devices->count() > 0);

This is now used:
| if(localmodel.fileInfo(index).isFile())
|     download_possible = trackmodel.is_open();

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

Okay, I fixed this.

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

Okay, reverted this one.

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

Damn, forgot to prefix that function, fixed.

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

In the new himd_set_track_label patch there is now a function to properly
delete strings, which himd_set_track_label now calls before adding the new
string.

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

label_type is now an enum and is checked using g_return_val_if_fail.

> The himd_modify_track approach is quite dangerous, as it relies on the
> user to pass the right track index to it.

Okay, the patch now introduces himd_start_update_track_info and
himd_commit_update_track_info like you suggested.

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

Okay, prints an error on failure now.


Kevin

Attachment: binF61Femy9iS.bin
Description: Binary data

Attachment: 0002-Activating-download-button-if-appropiate.patch
Description: Binary data

Attachment: 0003-Moved-get_songinfo-from-himdcli-to-libhimd.patch
Description: Binary data

Attachment: 0004-himd_set_track_label-implemented.patch
Description: Binary data

Attachment: bin5BdJsmUxAr.bin
Description: Binary data