Thanks for the comments. I'll convert them all into tasks or add them as comments in Gerrit if the patches in question are already in there.
On 02/06/2013 03:04 AM, Thiago Macieira wrote: > -2 on the -openssl-source option. It's not used anywhere. Yes, that's a left-over. I'll remove it. >> A lib/rules.xml > I'd rather you found a better place for this file. I was planning on putting it into src/android and having it deployed into lib/rules.xml when you build for Android. Would that be acceptable? I'd like to deploy it in lib/ because then the Qt Creator plugin doesn't need to look in two different places for the file for Qt 4 and Qt 5 builds. > . >> A mkspecs/features/java.prf > Ossi to judge if the mkpath should be there. Ossi wrote the code ;) > > javac.depends looks like a horrible hack. In fact, while this is a nice > feature, the whole file looks like a hack. qmake should be modified to learn > how > to compile Java instead. I disagree. The fewer hardcoded things we have in qmake, the better in my opinion. I do agree that the LINK override is hacky, but at least it's a working solution with the current system and the cleanest we could think of. If qmake is changed later to add a less hacky way to override the different compile steps, I'll be glad to adapt to it, but for now, I'd like to keep this build system and focus on more pressing things before the feature freeze :) > . Also, doesn't qpa/android.prf conflict with android.prf? I don't know. I haven't seen any issues. But I don't know if qpa/android.prf is actually used for anything. >> A src/android/... > I suggest the dir be renamed to src/androidmain, to match winmain and the old > (and thankfully removed) s60main. Ok, sounds good to me. > >> M src/corelib/kernel/qeventdispatcher_unix.cpp >> M src/corelib/kernel/qeventdispatcher_unix_p.h > This one needs a very good explanation. Would you mind discussing this with Bogdan on https://codereview.qt-project.org/#change,46798 Apparently it was needed to fix a deadlock in the event loop on Android. It was suggested that we move the hacky implementation into the android plugin instead. Would that be an acceptable solution, at least for a first go? > > M src/network/ssl/ssl.pri > -1: this unconditionally builds OpenSSL support in, even if the sources aren't > present. The configure script changes required for this aren't correct. I've already reverted the changes in SSL.pri. I'll remove the enablers from configure as well. > . > >> A src/widgets/styles/json/json.cpp >> A src/widgets/styles/json/json.g >> A src/widgets/styles/json/json.h >> A src/widgets/styles/json/json.pri >> A src/widgets/styles/json/jsonparser.cpp > -1. Do not add. > > You need to show that this is at least 5x faster than the one in QtCore before > this begins to be acceptable. > > I'm not the QtWidgets maintainer. If I were, I'd be giving a -2. Yes, I'll remove these and rewrite to use Qt's json parser. -- Eskil _______________________________________________ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development