kde review kartesio

[LucaTringali]

Hi, 
I'll answer point to point:

>----Messaggio originale----
>Hey!
>
>A good thing, I think such a tool could be useful to me too (and I
>know a lot of other people to whom it might be useful). Here's what I
>noticed from a quick look (some has been said already I think):
>
>* You probably shouldn't track the kdev4 file in the repository, same
>goes for screenshots

Yes, those files are there just because I found them useful, but I can remove 
them without any problem.

>* zorbaneural is a very fancy dependency, it's not even in arch's AUR.
>You should put the git URL into the cmake message.
>

I know, I wrote where to find it in Kartesio project overview but, since 
almost everybody had some troubles with it, I will make this thing more clear.

>* I put x**2 into the fit box and clicked Fit, and it crashed:
>http://paste.kde.org/741026/

Yes, the correct way to express a power is ^. So you should write x^2. There 
should be a check routine to avoid that a dangerous string like "**" is used, 
and I'm surely integrating this check in the next release.

>* Did you think about laying out the UI around a splitter? On my
>screen, the table takes most of the area and the plot is quite small,
>and I can't change that...

This could be a good idea: another thing for the next release.

>* It would be useful to be able to import data in some way, e.g. from
>a CSV file. I don't see a way to get data into the program except
>typing every number into the cells -- or is there another way? If
>there is, could it be made more obvious eventually?
>
Actually there is not: I'm working on a new window for the next releases: 
basically, there will be a button over the table, something like "Edit datas". 
This will open a new window in which it will be possible to import/export CSV, 
sort X axis values, add other rows or deleting some...

>* What does the code in calculations.cpp:117 do? It looks quite
>curious. Isn't there a more elegant solution (it looks a bit like a
>QChar::isLetter() implementation)?
>
>* calculations.cpp:505 and 584 the same code like in 117 again? It's
>weird enough to have that stuff once, but copied multiple times is bad
>imho ;)

No, at least not only. Originally, line 117 and 118 were collapsed into one 
single if instruction, it slip it in two because it was too long. This line 
checks if the current char (which is a C++ char an not a QChar) is permitted or 
not. Permitted characters ar letters, numbers, and some other simbols (for 
example "+", "(", etc..).
Could this instruction be shorter and more elagant? Probably. But it works, 
and actually I think it could stay as it is.

>* Your code uses mixed tab- and space indent (sometimes it uses tabs,
>sometimes spaces for no apparent reason). Most KDE apps use only
>spaces, you might consider if you want to do that too. Sometimes, the
>indent is even missing completely; you should indent one level after
>each opening curly parenthesis.
>* Same goes for the whole formatting of the code, it's pretty
>inconsistent. For example, look at the spaces around operators or so.

I know it, I'll try to make the code more readable, but I do not have so much 
time so usually I prefer to dedicate my time to new features or corrections 
instead of making them prettier.

>* Instead of writing to /tmp/kartesiotmp.txt you should probably use
>QTempFile. That will also take care of the deleting the temp file when
>it gets deallocated so you don't need to exec (scary and
>platform-dependent) rm commands.

I'm already working with QTempFile for the next release of Kartesio.

>* calculations.cpp:277 this makes no sense, there's a statement behind
>a "return"
>
Ooops: I thought I already removed it. 

>In general, you're mixing a lot of plain C / stdlib stuff into Qt
>code. Is there a reason for that? For example, in calculations.cpp:148
>you take text from a text field, convert it to a byte array, convert
>it to a char* and then pass it to a function. Why not just pass the
>QString? You can iterate over a QString like
>foreach ( const QChar& c, myqstring ) { ... }
>or also
>for ( int i = 0; i < myqstring.size(); i++ ) { ... }
>if you like that better, and you can also index it like a char*, as in
>mystring[i+1] or so.

Yes, this is an heritage from the older version of Kartesio, that was based 
mainly on plain ANSI C++. Those mixing are just  an hack to make Kartesio work 
immediately. If I'll have time, I will "translate" everything into Qt, but 
first of all I would like to apply other features.

>Also, nothing in your code is const and everything is public, although
>almost everything could be const and private, but I won't get started
>on that now ;)
>
>This is not meant as a list of what you must fix, it's just my two cents.
>

Thank you, it's always nice to get some suggestions.

>Cheers!
>Sven
>
>2013/5/10 Tomaz Canabrava <tcanabrava at kde.org>:
>> Annma, I find that proposal *very* good.
>>
>> I'm a bit distant of KDE programming - I know - because my day job is 
making
>> me work 12h+ creating scientific tools.
>> ( actually - one of the tools that I created here was a... Solver, to fit
>> curves on points... )
>>
>> Tomaz
>>
>>
>> 2013/5/10 David Edmundson <david at davidedmundson.co.uk>
>>>
>>> The app sounds awesome.
>>>
>>> From the application life cycle page you linked:
>>> > When you have made one of more releases and want to continue to develop
>>> > it, the term 'playground' does no longer apply to you. That is the right
>>> > time to move out of here
>>>
>>> There are no releases on download.kde.org under unstable. Have you
>>> made these releases elsewhere? If so can you provide a link.
>>>
>>> Thanks
>>>
>>> David Edmundson
>>
>>
>