Ticket #300

[Dirk Hohndel]

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