Hi, Mariano,
After all this is done and hopefully merged to main line,
On Sat, Jun 21, 2014 at 5:48 PM, Mariano Reingart <rein...@gmail.com> wrote:
> FYI
>
> The last issues to get the controls sample working where fixed:
>
> * Fixed wxMemoryDC object de-selection
> * Fixed DrawEllipse fill
> * wxFont set face name
>
> Screenshot (very similar to wxGTK):
>
> https://cloud.githubusercontent.com/assets/1041385/3350335/e2b38286-f9a4-11e3-991b-48a91d46d910.png
>
> All commits in
>
> https://github.com/reingart/wxWidgets/commits/SOC2014_QT
>
> I'm moving to the final ticket #5 prior mid-term (testing, checks,
> documentation, etc.)
what would be the default toolkit on *nix: wxGTK or wxQT?
Meaning that if I just issue "../configure" - will I be building wxGTK or wxQT?
Thank you.
MR> As I did some work in advance, I think I'll be able to create a new
MR> "future" unplanned milestone for the issues discovered during this gsoc
MR> (almost all introduced in prior gsoc):
MR>
MR> * Refactor to use RAII (VADZ)
This would be great but it's not really critical.
MR> * Clean up (remove Qt from wxBase and warning) (VADZ)
This is the most important thing IMO. We really should have the same
wxBase for all (Unix) ports.
MR> * Memory leaks / problems reported by Valgrind
This looks serious too, it seems that not much is being freed right now.
MR> * Revise naming conventions
MR> * Revise internal headers
And this is important as well to make it simpler for other people to make
on this port.
Another item I'd really like to see would be to make the unit tests for
the already implemented controls to run -- and to pass -- for wxQt. This
would give us a solid foundation for the future work, otherwise we would
never know when changing something somewhere else breaks wxQt.
On Wed, 2 Jul 2014 02:59:18 -0300 Mariano Reingart wrote:
MR> I'm more confident I could finish "wxQT for Android" milestone quickly and
MR> then work with the unplanned issues (and not the other way).
MR> In fact, today I have to "fix" a new compilation issue and found a SIGSEGV
MR> fault just to try to run the tests (see bellow).
But it's much more important to fix this segfault so that people could try
to use wxQt without running into fatal problems immediately than to have
a half working (because it would still suffer from the same bug(s),
wouldn't it?) wxQt/Android port.
There are two conditions for merging into the trunk. The first one is that
that whatever is done in the port, should be done well. I.e. it's *not* a
problem if the port is incomplete (so all items with "all" in them are not
necessary). But it is a problem if the already implemented doesn't work
correctly (and this includes crashes and memory leaks) or is not
architectured correctly (this includes wxBase problem). IOW, it should be
possible to work on the port in the future by simply adding things to it,
and not by having to rewrite it.
The second condition is that it should be minimally usable. I.e. if the
port is very well-written but only implements wxStaticText and nothing
else, it wouldn't be considered to be ready for the merge neither. But I
think wxQt is quite beyond this point (even though having wxBookCtrl,
wxListCtrl, wxTreeCtrl and wxGrid would be definitely very nice in
practice), which is why I concentrate more on the previous one. Again, we
can always add things later, but we need to have a solid foundation to
build them on.
I can assure you that it won't be and that if you continue to work on this
project as well as you have done so far, you will have absolutely no
problems with the final evaluation.
MR> Finally, I really would like to get wxPython minimally working for wxQT
MR> (and Android) as planned
I don't say it wouldn't be nice, but, again, IMO this is much less
important than working on wxQt core itself. For example, I'm pretty sure
that if wxQt is merged into the trunk, somebody will build wxPython with it
(and sooner rather than later). OTOH I'm far from sure that somebody will
spend time on refactoring and debugging wxQt if it is not merged by the end
of this project. And merging it is really the important thing here, if we
can't do it, the project will be a failure from my point of view (i.e. not
necessarily from the GSoC evaluation one, I accept that you could be
working hard on it and still not manage to achieve it).
MR> > MR> * Memory leaks / problems reported by Valgrind
MR> >
MR> > This looks serious too, it seems that not much is being freed right now.
MR> >
MR> Sorry, I don't fully understand this one.
MR> According to valgrind report, there are just 4 different focus of memory
MR> leaks and 1 uninitialized value.
MR> Also, the wxQtPointer helper should destroy the no-longer-used instances.
MR> Surely I'm missing something, and to be honest, I fell my C++ skills are
MR> not that advanced to proceed here.
Hunting memory leaks is more about spending enough time than any skills
I'm afraid. When you get a leak, such as this one:
https://github.com/reingart/wxWidgets/issues/24
for example, you need to find out why isn't the memory freed in the place
it's supposed to be. In this particular case, it's supposed to happen in
wxFrameBase::DeleteAllBars(), so it's probably just the case of wxFrame
dtor not calling it -- and looking at the code in src/qt/frame.cpp, this is
indeed the case.
Now the strange thing is that I have no idea why do we duplicate the call
to this method (and SendDestroyEvent()) in all the derived classes dtors.
So instead of adding this line to wxFrame dtor in wxQt, it would actually
be better to just stop duplicating this code and I'll do this change in the
trunk in a moment. So all you'll have to do to fix this leak would be to
merge in the latest trunk changes.
See, this was easy. Others will be less so, of course, but it's usually
still not very difficult to find leaks (at least without any reference
counting being used, this always makes things more interesting). If you
don't know where something is supposed to be freed, please ask. But you
should spend time on tracking them down, if only to ensure that we're not
doing something particularly stupid.
MR> This is also something that worries me, if I embark to fix this kind of
MR> issues, they could take me a lot of time and I wouldn't be as much
MR> productive for this project as I could be following the original plan (IMHO)
But they're very important... If there is some fundamental problem
resulting in memory leaks, this means that wxQt just can't be used for
anything at all (including wxPython apps, it's not like the memory is
going to magically clean up itself just because you add an extra layer on
top of the code leaking it).
MR> > Another item I'd really like to see would be to make the unit tests for
MR> > the already implemented controls to run -- and to pass -- for wxQt. This
MR> > would give us a solid foundation for the future work, otherwise we would
MR> > never know when changing something somewhere else breaks wxQt.
MR> >
MR> Yes, this would be great but I will need some assistance here.
MR> I tried to find some documentation about wx testing in this scenarios
MR> without luck.
I guess you already saw
https://github.com/wxWidgets/wxWidgets/blob/master/docs/contributing/how-to-write-unit-tests.md
Do you have any precise questions not addressed there?
MR> (Very) preliminary, half of the test seems to pass, but the other half fail
MR> or raises error, until a SIGSEGV on widget destructor:
MR>
MR> https://github.com/reingart/wxWidgets/issues/29
This probably has something to do with wxBookCtrl being not implemented?
I.e. I guess that something is wrong with the parent of the book page?
Anyhow, if this class is known not to work you can just skip its tests
(put "#ifndef __WXQT__" around them). But it would be great if the test
suite could pass.
MR> Best regards and sorry for the long mail (and please pardon me if I was not
MR> clear due to my English)
I'm afraid I understand you perfectly :-) I do realize that working on
wxPython/Android is more exciting than hunting memory leaks or refactoring
wxBase. But I still think that the latter is much more important for the
future chances of success of wxQt and I'd still very much prefer you to
deal with these problems first so that we could be certain to be able to
merge your work into the trunk at the end.
On Sun, 17 Aug 2014 21:24:54 -0300 Mariano Reingart wrote:
MR> I'm working on the documentation, updating and enhancing the wxQT wiki page:
MR>
MR> http://wiki.wxwidgets.org/WxQt
I'd move the "internals" part of this into docs/qt/internals.md, wiki is
user-oriented (we do have developer wiki at http://trac.wxwidgets.org/wiki
but it's not used much).
MR> http://wiki.wxwidgets.org/WxQt/Status
This is very useful, thanks. We probably need to have all this information
in the manual actually, but it shouldn't be difficult to move it there
when/if someone finally finds time to do it.
MR> I've also added some install instructions to the docs folder:
MR>
MR> https://github.com/reingart/wxWidgets/tree/SOC2014_QT/docs/qt
This still mentions GTK+ version which is probably not very important for
wxQt bug reports.
MR> It would be great if someone could test the build steps and proof-read the
MR> texts, and send me any error, correction or suggestions to improve it.
I just rebuilt it again without problems under Debian with Qt 5.3. There
are some warnings, for the reference:
----------------------------------------------------------------------------
Makefile:20544: warning: overriding recipe for target 'monodll_converter.o'
Makefile:20541: warning: ignoring old recipe for target 'monodll_converter.o'
Makefile:20847: warning: overriding recipe for target 'monodll_qt_utils.o'
Makefile:20844: warning: ignoring old recipe for target 'monodll_qt_utils.o'
Makefile:26406: warning: overriding recipe for target 'monolib_converter.o'
Makefile:26403: warning: ignoring old recipe for target 'monolib_converter.o'
Makefile:26709: warning: overriding recipe for target 'monolib_qt_utils.o'
Makefile:26706: warning: ignoring old recipe for target 'monolib_qt_utils.o'
src/common/utilscmn.cpp:1064:56: warning: unused parameter 'flags' [-Wunused-parameter]
bool wxDoLaunchDefaultBrowser(const wxString& url, int flags)
^
src/qt/checkbox.cpp: In member function 'virtual wxCheckBoxState wxCheckBox::DoGet3StateValue() const':
src/qt/checkbox.cpp:122:1: warning: control reaches end of non-void function [-Wreturn-type]
}
^
src/qt/cursor.cpp:135:48: warning: unused parameter 'data' [-Wunused-parameter]
wxCursor::CloneGDIRefData(const wxGDIRefData * data) const
^
src/generic/graphicc.cpp:1890:73: warning: unused parameter 'window' [-Wunused-parameter]
wxCairoContext::wxCairoContext( wxGraphicsRenderer* renderer, wxWindow *window)
^
src/generic/graphicc.cpp:2244:60: warning: unused parameter 'text' [-Wunused-parameter]
void wxCairoContext::GetPartialTextExtents(const wxString& text, wxArrayDouble& widths) const
^
----------------------------------------------------------------------------
The make warnings are probably bakefile bug (?) but if you could find a
way to get rid of them, it would be nice, it's going to be annoying to have
them during each rebuild. The unused parameters warnings are, of course,
trivial, but still worth working around by adding "WXUNUSED()". Finally,
the warning in checkbox.cpp is serious as other compilers would give an
error and not a warning here, please fix it.
Running the minimal sample spits out a kit if debug output which should
really be disabled. Please use wxLogTrace("qt.whatever", ...) instead of
wxLogDebug(...) for the things you really want to keep, it would then be
possible to turn them on by setting WXTRACE environment variable to
"qt.whatever" but they wouldn't be given by default.
There are also things which don't seem to come from wxLogDebug():
----------------------------------------------------------------------------
src/qt/statusbar.cpp(41): Missing implementation of "wxStatusBar::Create parameters"
libGL error: failed to load driver: swrast
QXcbConnection: XCB error: 147 (Unknown), sequence: 162, resource id: 0, major code: 144 (Unknown), minor code: 20
----------------------------------------------------------------------------
The latter could be the result of running it over a remote X11 connection,
if it doesn't appear when running locally, it's probably not worth
bothering with.
Running the minimal sample works well (as mention in the status page the
accelerators don't work), but the about dialog appears in the upper left
corner of another display instead of appearing on the one I'm running on
for some reason. Again, this is rather minor, of course, but I'm just
surprised that this, of all things, doesn't work correctly, I'd expect Qt
to take care of it.
Running the dialogs sample also works well, although it's amazing how out
of place all Qt copies of the standard Windows dialogs look under Linux,
but it crashes on exit. Unfortunately I don't have time to debug this, but
it seems that something is still wrong with the window destruction :-(
Drawing sample works mostly well until it's scrolled. Once you do it, the
display is broken and the program also crashes on exit (maybe this is
unrelated however, again, I tested this only very quickly). Scrolling
problem is worth mentioning on the status page.
All in all, it's amazing how many things work, but there are still a few
important ones which don't. I'm mostly worried by the continuing problems
with the window destruction, it's something that we really must get right
once and forever. If you could continue looking into this after the end of
the GSoC, it would be great.
Please let me know when would you like this branch to be merged. AFAICS
there are no non-trivial changes in the files outside of include/wx/qt and
src/qt, so I see no problems with merging this at any moment.
Thanks for your great work!
On Mon, 18 Aug 2014 23:30:04 -0300 Mariano Reingart wrote:
MR> > MR> http://wiki.wxwidgets.org/WxQt
MR> >
MR> > I'd move the "internals" part of this into docs/qt/internals.md, wiki is
MR> > user-oriented (we do have developer wiki at http://trac.wxwidgets.org/wiki
MR> > but it's not used much).
MR>
MR> Ok, all the "Architecture" section or just "internals" subsection?
MR> I think it will make sense moving all that section, as topics are related.
Yes, I agree.
MR> > I just rebuilt it again without problems under Debian with Qt 5.3. There
MR> > are some warnings, for the reference:
MR> >
MR> > ----------------------------------------------------------------------------
MR> > Makefile:20544: warning: overriding recipe for target 'monodll_converter.o'
MR> > Makefile:20541: warning: ignoring old recipe for target
MR> > 'monodll_converter.o'
MR> > Makefile:20847: warning: overriding recipe for target 'monodll_qt_utils.o'
MR> > Makefile:20844: warning: ignoring old recipe for target
MR> > 'monodll_qt_utils.o'
MR> > Makefile:26406: warning: overriding recipe for target 'monolib_converter.o'
MR> > Makefile:26403: warning: ignoring old recipe for target
MR> > 'monolib_converter.o'
MR> > Makefile:26709: warning: overriding recipe for target 'monolib_qt_utils.o'
MR> > Makefile:26706: warning: ignoring old recipe for target
MR> > 'monolib_qt_utils.o'
MR> >
MR>
MR> I don't understand why they are happening, and even Sean had the same doubt.
MR> Anyway, I will try to review it when I have some time, but if you could be
MR> more specific, that would be great to solve it faster.
I'll look at it.
MR> > src/qt/checkbox.cpp: In member function 'virtual wxCheckBoxState
MR> > wxCheckBox::DoGet3StateValue() const':
MR> > src/qt/checkbox.cpp:122:1: warning: control reaches end of non-void
MR> > function [-Wreturn-type]
MR> > }
MR> > ^
MR>
MR> Ok, I'll review the code of Sean as it contributed that part.
MR> At first, it seems harmless as the switch covers the three enum values:
MR> Unchecked, PartiallyChecked, Checked
MR> I prefered to left this kind of situations undecided by now instead of
MR> adding boilerplate or wrong code.
Not really a lot of boilerplate, but you do need a return even if it's
unreachable, see http://www.wxwidgets.org/develop/coding-guidelines/#no_default_in_switch
MR> > src/qt/cursor.cpp:135:48: warning: unused parameter 'data'
MR> > [-Wunused-parameter]
MR> > wxCursor::CloneGDIRefData(const wxGDIRefData * data) const
MR> > ^
MR>
MR> This one I have no idea why Sean commented it out:
MR> https://github.com/reingart/wxWidgets/commit/9cc52903c22186b925055de22b6c1a0b68b8dc11
Err, yes, but you shouldn't merge the changes you disagree with in your
tree. Looking at the code, this is actually much worse than an unused
parameter warning because this means that cursors can't be copied at all.
Worse, looking at CreateGDIRefData() above, I see that it was commented out
as well and returns NULL and I think this can just make some common code
crash. Or, in the best case, fail to work.
MR> > src/generic/graphicc.cpp:2244:60: warning: unused parameter 'text'
MR> > [-Wunused-parameter]
MR> > void wxCairoContext::GetPartialTextExtents(const wxString& text,
MR> > wxArrayDouble& widths) const
MR> > ^
MR> >
MR>
MR> This is not implemented for any other port than wxGTK
MR> I cannot put a WXUNUSED as it will break gtk build (same in the previous
MR> one)
You can use wxUnusedVar() inside #ifdef __WXQT__.
MR> I never run this in a remote console, and dialogs / windows allways shows
MR> in the default position.
MR> In wxGTK, the about dialog open in the middle of the parent window, should
MR> be this the correct position?
I think so.
MR> Again, the crash is something sporadic and could be not related to wxQT,
I'm pretty sure it's related to wxQt, there is no way to make it crash
when using wxMSW or wxGTK.
MR> As I said in previous emails, sadly fixing all bugs or complex issues was
MR> too extensive (for example, this year other ports had specific GSOC
MR> projects to fix unit tests or develop a GDI layer)
Yes, this was an ambitious project, clearly. I must say that
MR> Yes, please merge it
It turns out there is a problem with doing it while preserving the history
of your changes :-( I wanted to. I realize now that we had made a big
mistake with not creating the branches in the svn first, but, well, it's a
bit too late to do anything about this.
My hope was to "git rebase --preserve-merges" your branch on the newly
created SOC2014_QT but this stopped repeatedly with a lot of failed merges
and I didn't have the courage to manually resolve them after the second
one. So I tried to at least commit your work to the old wxQT branch which
then would be merged into the trunk but this failed because of svn
properties mismatch for the binary files in wxQT branch and I can't fix
this without Robin's help so it will have to wait until he can help me.
I hope to resolve this soon but this doesn't depend only on me,
unfortunately.
In any case you shouldn't work in your Github fork any more as the
branches have diverged now. However you can commit either to svn itself (at
least to wxQt files) or create new branches for the individual changes on
Github and submit pull requests that I would then apply. Please let me know
what do you prefer.