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