On Sat, 2014-03-01 at 04:06 +0200, Nicu Badescu wrote: > This is an attempt to solve ticket #300 > (http://trac.hohndel.org/ticket/300).
Hi Nicu, thanks for submitting these three patches. Here are a few comments: a) please read our CodingStyle document. Your newly added code was indented with 4 spaces b) the dialog that you added looks very cramped (certainly at the font size I happen to use part of the text is cut off) - if you look through the existing code, we never use QInputDialog anywhere else c) you use toLocal8bit() to create a QByteArray and then pass the data() of that QByteArray to the C function. Why the intermediary step into a QByteArray if you don't use that for anything else. Simply use BookmarkName.toUtf8().data() as argument to the C function (oh, and yes, use toUtf8() as that's the only encoding that we allow in the XML file) d) you added this to the old profile code (but I can understand why - the bookmark code in the new profile appears to be only partly implemented... I need to figure out what happened there). But the problem with that is that we are planning to remove the old profile within the next few days. Maybe you could figure out how to hook up the bookmark code in the new profile and add your code there? e) please check your commit messages. they are somewhat hard to understand f) (this is nit picking) - you might have combined the first two commits into one... but that's not really required... we have many instances in our git history that are similar to your patches in that one commit adds something and the next commit fixes an oversight in the previous commit. In general it might be better to just squash the two together, especially with something as trivial as your second patch Thanks for working on this! Please use these comments to create a better patch series. /D _______________________________________________ subsurface mailing list [email protected] http://lists.hohndel.org/cgi-bin/mailman/listinfo/subsurface
