Le mercredi 30 juin 2010 à 20:30 -0400, M Gagnon a écrit :
> Hi Xapantu,
>
> thanks for the great work =D
> I quickly looked at the code. Here are a couple remarks
Thanks !
>
> * I saw a few comments "the unsigned is to remove the compiler warnings,
> maybe it is a bad idea ?". Not this is no problem ;)
> Since the variable in the for loop can never be negative.
>
> * You have many short functions like this :
> std::string Addons::GetName()
> {
> return this->addons_list[this->index].name;
> }
> Those one-liners can be placed directly in the .h file, this way the
> compiler can inline them (it makes them a bit faster). But
> your work was not technically incorrect either!
ok, I am going to fix that.
> * The Addons screen AND the Addons Update screens header both contained
> this :
> #ifndef HEADER_ADDONS_SCREEN_HPP
> #define HEADER_ADDONS_SCREEN_HPP
> This prevents from including the two headers :) Probably some bad copy
> and paste, I fixed it.
oh, yes, it is an error, I don't start to work on the update system...
> * I added a "#ifdef ADDONS_MANAGER" around the addon update screen, it
> could not compile
>
> * I generally see you use "this->x". I think this comes from python
> self.x =) While using "this->x" in C++ is definitely not
> bad in itself, the STK coding guidelines are to not use "this" and
> instead prefix the name of member variables with m_. So
> "this->x" becomes "m_x", and "this->foo()" becomes "foo()". Not very
> important though, I prefer working code above all ;)
yes, it comes from python... I didn't suppose that we could do otherwise,
I am going to fix that too.
> * Some more nitpicking on coding style ;)
> if ( S_ISREG(file_stat.st_mode) ) {
> remove(buffer);
> }
> else if ( S_ISDIR(file_stat.st_mode) ) {
> this->RemoveDirectory(buffer);
> }
> according to our coding guidelines should be :
> if ( S_ISREG(file_stat.st_mode) )
> {
> remove(buffer);
> }
> else if ( S_ISDIR(file_stat.st_mode) )
> {
> this->RemoveDirectory(buffer);
> }
>
> * On line "FILE * fp = fopen(std::string(file_manager->getConfigDir() +
> std::string("/") + file).c_str(), "w");"
> You need to perform error handling
> if (fp == NULL)
> {
> // warn the user that creating the file failed
> return;
> }
> Same thing for the curl functions, make sure to check the return values
> to know if there are errors =)
I think there will be a lot of work to handle this error (e.g. if we
haven't any connexion... :s)
> * Still regarding the "download" functions : there are two download
> functions with almost identical contents. You
> know this is bad ;) instead make one call the other with a default
> argument, that will be cleaner.
yes, I didn't think to this.
> * Regarding error handling :
> if (IFileSystem_copyFileToFile(dstFile, srcFile) < 0)
> ; // error
> I think this is incomplete code, but I think it should at least be
> marked with a FIXME so it's not forgotten in this state ;)
>
> * There is a memory leak starting here :
> "GUIEngine::ListWidget* w_list =
> this->getWidget<GUIEngine::ListWidget>("list_addons");"
> I believe "w_list" is never deleted.
>
> * "pt->update_status->setText(_("Updating the list..."));" is called
> from within a thread; this is not correct since our
> GUI toolkit is not thread safe. In this particular case I think it would
> be simplest to just update the text from the main
> thread before or after launching the other thread. (I don't think the
> thread access will cause problems in this particular
> case, but better not play with fire)
>
> * Things to think about regarding threads : what happens if the user
> cancels/leaves the screen before the thread is over?
> Is there a possibility that by some eager combination of actions (e.g.
> repeatedly clicking) the user could start more than
> one thread executing simultaneously?
Yes, there is some problem with the threads, I will try to fix them.
> * In the update, function "void * startInstall(void* pthis)" does many
> thread-unsafe accesses. For instance, loadInformations()
> is called by both the main thread and the secondary thread. That can
> create nasty clashes; and even if both didn't access it,
> making GUI calls from other threads is not supported anyway. So ideally
> the GUI should be updated in the main thread
The update screen is just a quick copy paste of the addons screen, I didn't start it.
> But besides this long list of comments, good job =D
> I have not yet tried the code (it doesn't build anyway), I've only read
> it, so this is to be continued :)
>
> -- Auria
Thanks a lot for your feedback !
xapantu
|
