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