GSoC 2014 wxQT progress/plans

157 views
Skip to first unread message

Mariano Reingart

unread,
Jun 16, 2014, 3:28:15 PM6/16/14
to wx-dev
FYI (I'll summary the comments discussed in GitHub)

Last week #4, after merging the experimental branch to remove moc (see the related thread), I worked on getting critical controls working minimally for wxQT.

Now, all tabs of controls sample show ok and several issues were solved (mostly problems with sizing and control creation)

Screenshots and commit history in:

https://github.com/reingart/wxWidgets/issues/4

Most important remarks are:
 * wxTextCtrl implementation and wxQt helper should be rethinked (it was using QTextEdit but it is multiline, QLineEdit need to be considered)
 * QLayout are mandatory in some places (wxRadioBox), qt sizing hints has some semantic differences with wx (and some corner cases like minimun hint being greater than normal one)
 * Styling issues: ubuntu seems to be appling a custom theme and some controls do not render as expected (and also prevents changing properties like background), what about qApp->setStyleSheet (css-like)?
 * DC drawing doesn't seems to be working correctly (at least for bitmap button)

Updated current status of controls minimally working on wxQT:

 * wxNotebook, wxStaticBox and wxRadioBox (containers)
 * wxRadioButton, wxCheckBox
 * wxChoice / wxComboBox, wxListBox. 
 * wxTextCtrl (multiline and single line)
 * wxStaticText (now also word wrap)
 * wxButton, wxBitmapButton, wxToggleButton
 * wxSlider, wxGauge, wxSpinCtrl

For this week, I'll work on get the basis working (painting, scrolling, and adding missing methods/events to critical controls):


In the next week (mid-term evaluation), I'd like to do some basic tests and review:


Advice and comments are welcome,

Best regards,

Mariano Reingart

unread,
Jun 21, 2014, 6:43:10 AM6/21/14
to wx-dev
FYI 

Update: 

Controls sample almost work (usable like in wxGTK), summary of last changes:

 * wxTextCtrl: write, get value, selection, insertion point, etc. (single & multiline)
 * wxStaticText: set label 
 * wxToggleButton event
 * wxWindow enable
 * wxCheckBox event
 * wxListBox events, string methods, selection, clear/deletion, insert, item data
 * wxChoice event, basic methods
 * wxComboBox event, basic methods, edition & text event
 * wxRadioBox/wxStaticBox helper, event
 * wxGauge stub, basic methods
 * wxSlider event, basic methods
 * wxSpinButton helper (& rename fix)
 * wxSpinCtrl helper "polymorphic", event

I've added wxAnyButton header and code in a refactory (extracted from wxButton)
Also I've remove shared header combobox_qt and groupbox_qt
I started to use internal thin wxQt helpers for each control (just to connect the events and avoiding to touch headers too much), in a attempt for simplification & to accelerate the development, 

All commits in 

The only remaining major issue (for the controls sample) is the DC bitmap button drawing (I'll focus on this)

Advice is welcome, fell free to comment on the commits or open tickets

Mariano Reingart

unread,
Jun 21, 2014, 8:49:01 PM6/21/14
to wx-dev
FYI

The last issues to get the controls sample working where fixed:

 * Fixed wxMemoryDC object de-selection 
 * Fixed DrawEllipse fill 
 * wxFont set face name 


I'm moving to the final ticket #5 prior mid-term (testing, checks, documentation, etc.)

Igor Korot

unread,
Jun 21, 2014, 8:57:01 PM6/21/14
to wx-dev
Hi, Mariano,

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.)

After all this is done and hopefully merged to main line,
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.
> --
> To unsubscribe, send email to wx-dev+un...@googlegroups.com
> or visit http://groups.google.com/group/wx-dev
>

Mariano Reingart

unread,
Jun 21, 2014, 9:09:55 PM6/21/14
to wx-dev
On Sat, Jun 21, 2014 at 9:57 PM, Igor Korot <ikor...@gmail.com> wrote:
Hi, Mariano,

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.)

After all this is done and hopefully merged to main line,
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.

wxGTK is much more advanced than wxQT and it seems to be the "default" in many GNU/Linux distros.

wxQT will still need work after the eventual merge, there are some corner cases and many features that still need to be implemented.

IMHO hopefully the more complete, updated and documented wxQT (in what I'm working on) would improve and could speed up its development.
I think it will be definitively useful to experiment in other platforms (ie. android) and looking forward to new alternative scenarios (ie ubuntu sdk)

Mariano Reingart

unread,
Jun 27, 2014, 11:55:15 PM6/27/14
to wx-dev
Midterm progress: 

Second planned milestone was completed, please see issue and commits on GitHub:



Changes (summary):

 * Made qt helpers internal and thin (almost no trivial methods)
 * Fixed calendar control
 * Fixed check list control
 * Completed functional implementation of text control (events)
 * Refactorized combobox and buttons
 * Fixed notebook events (better implementation)
 * Fixed SIGSEGV on paint event
 * Template helper moved to private header
 * Revised prior commits (ie new wxGA_TEXT style)
 * Added and fixed slider ticks
 * Minor fixes and improvements

Next week will start with the last planned milestone (wxQT for android):


As I did some work in advance, I think I'll be able to create a new "future" unplanned milestone for the issues discovered during this gsoc (almost all introduced in prior gsoc):

 * Refactor to use RAII (VADZ)
 * Clean up (remove Qt from wxBase and warning) (VADZ)
 * Memory leaks / problems reported by Valgrind
 * Revise naming conventions
 * Revise internal headers

For more info see:


Best regards

Vadim Zeitlin

unread,
Jun 30, 2014, 8:21:48 AM6/30/14
to wx-...@googlegroups.com
On Sat, 28 Jun 2014 00:54:51 -0300 Mariano Reingart wrote:

MR> Midterm progress:
MR>
MR> Second planned milestone was completed, please see issue and commits on
MR> GitHub:
MR>
MR> https://github.com/reingart/wxWidgets/issues/5
MR>
MR> https://github.com/reingart/wxWidgets/commits/SOC2014_QT
MR>
MR> Changes (summary):
MR>
MR> * Made qt helpers internal and thin (almost no trivial methods)
MR> * Fixed calendar control
MR> * Fixed check list control
MR> * Completed functional implementation of text control (events)
MR> * Refactorized combobox and buttons
MR> * Fixed notebook events (better implementation)
MR> * Fixed SIGSEGV on paint event
MR> * Template helper moved to private header
MR> * Revised prior commits (ie new wxGA_TEXT style)
MR> * Added and fixed slider ticks
MR> * Minor fixes and improvements

All this is very nice, thanks!


MR> Next week will start with the last planned milestone (wxQT for android):
MR>
MR> https://github.com/reingart/wxWidgets/issues?milestone=3&state=open

To be honest, I don't really know whether it's better to proceed to this
directly or leave it until the very end and rather risking spending a lot
of time on it now and not managing to finish neither it, nor the items
below. Being cautious, I'd personally prefer to fix the desktop issues to
ensure that the wxQt branch can be merged into the mainline at the end of
the GSoC, as not working under Android wouldn't prevent it from happening
-- but other problems could.

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.

TIA!
VZ

Mariano Reingart

unread,
Jul 2, 2014, 1:59:40 AM7/2/14
to wx-dev
I'm more confident I could finish "wxQT for Android" milestone quickly and then work with the unplanned issues (and not the other way).
In fact, today I have to "fix" a new compilation issue and found a SIGSEGV fault just to try to run the tests (see bellow).

So, I'm afraid going deeper with "wxQT for desktop" could unveil more issues and it would be difficult to draw a line to determine where to stop given the deadlines (that's why I planned to work with the limited controls sample in the first place).

For example, I don't know which is the condition for "wxQT desktop" (or other ports) to be merged into mainline:
 * it should be "suitable for developing small applications"?
 * all (native) controls in core should work ok?
 * all controls in adv should work ok?
 * all samples should work?
 * all test should pass?
etc.

(the first is a statement in the GSOC projects idea page that I've used in my proposal)

I did a quick review these days and the current status is:
 * Most native controls works (at least the basics to develop "small applications" in my POV)
 * More or less other samples (like widgets and calendar) compiles and works, although YMMV
 * Some important "container" controls like wxBookCtrl don't work (hence widgets sample is not usable)
 * wxListCtrl / wxTreeCtrl samples have issues / don't compile / raise seg fault 
 * wxGrid sample have assertions and don't draw correctly
 * There seems to be still some assorted sizing issues
 * Generic controls (like for the calendar), still don't work due to drawing issues.
 * Tests are in a similar intermediate situation, see bellow.

Also, I don't want to cause any trouble with the GSOC evaluation process, as in my application I planned "wxQt for Android" milestone with several deliverables, and that was reviewed and approved. 
Maybe not completing it could be seen as a failure.

Finally, I really would like to get wxPython minimally working for wxQT (and Android) as planned, I think it would be more useful to experiment, giving me more insights of wxWidgets to help after the GSOC.
As wxPython could be a bit overwhelming initially (at least based on some attempts I did in the past), I would start testing the controls sample on Android and then you could evaluate how to proceed according the time left (I explicitly stated this in my proposal:  wxPython would be delivered if possible within the given time frame).
I already did packaged the minimal sample for Android using wxQT 2.9 so I don't see any major issue here, you could see my GSOC application for more info and prior work:


I think I could complete the 6 planned tickets before July 20 (or 27 at the latest), in 3/4 weeks, shrinking the schedule due the work done in advance, the experience learned and wxPython laid off if necessary.
Then, I could accommodate the unplanned milestone, with 13 tickets so far, in the 2/3 remaining weeks (I'll do my best but I'm not sure I could complete it).


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.


Ok
 
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.


I'm a bit lost here, where should I start to look to solve this?
 
MR>  * Memory leaks / problems reported by Valgrind

 This looks serious too, it seems that not much is being freed right now.


Sorry, I don't fully understand this one.
According to valgrind report, there are just 4 different focus of memory leaks and 1 uninitialized value.
Also, the wxQtPointer helper should destroy the no-longer-used instances.
Surely I'm missing something, and to be honest, I fell my C++ skills are not that advanced to proceed here.

This is also something that worries me, if I embark to fix this kind of issues, they could take me a lot of time and I wouldn't be as much productive for this project as I could be following the original plan (IMHO)
 
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.


Yes, and there is the "pencils down" last week to focus on this if I run off "regular" time
 
 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.


Yes, this would be great but I will need some assistance here.
I tried to find some documentation about wx testing in this scenarios without luck.
(Very) preliminary, half of the test seems to pass, but the other half fail or raises error, until a SIGSEGV on widget destructor:


BTW, I commited today a missing wxFont constructor using wxFontInfo that was preventing test to compile (missing implementation yet as I need to investigate how to use wxFontRefData in wxQT). 

Best regards and sorry for the long mail (and please pardon me if I was not clear due to my English)

Vadim Zeitlin

unread,
Jul 2, 2014, 8:39:40 AM7/2/14
to wx-...@googlegroups.com
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.

MR> For example, I don't know which is the condition for "wxQT desktop" (or
MR> other ports) to be merged into mainline:
MR> * it should be "suitable for developing small applications"?
MR> * all (native) controls in core should work ok?
MR> * all controls in adv should work ok?
MR> * all samples should work?
MR> * all test should pass?
MR> etc.

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.

MR> Also, I don't want to cause any trouble with the GSOC evaluation process,
MR> as in my application I planned "wxQt for Android" milestone with several
MR> deliverables, and that was reviewed and approved.
MR> Maybe not completing it could be seen as a failure.

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> * Clean up (remove Qt from wxBase and warning) (VADZ)
MR> >
MR> > This is the most important thing IMO. We really should have the same
MR> > wxBase for all (Unix) ports.
MR> >
MR> I'm a bit lost here, where should I start to look to solve this?

The goal is to not use any Qt APIs from wxBase code. I don't know where
are they used currently, you should look at all the places where this is
done and start a separate thread about this. But basically the solution is
almost invariably to virtualize things via wxAppTraits: this is the
mechanism which is used to allow the same class (or function) to work
differently in wxBase (console applications) and GUI applications.


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.

TIA,
VZ

Mariano Reingart

unread,
Jul 3, 2014, 3:28:36 PM7/3/14
to wx-dev
On Wed, Jul 2, 2014 at 9:39 AM, Vadim Zeitlin <va...@wxwidgets.org> wrote:
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.

I don't think so, it depends, for sample applications only using core native controls, there will be no fatal problems.
The segfault is only raised when running the gui tests (and hence hitting a lot of unimplemented features)
 
 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.

I don't want to open a debate, but I suffered some segmentation faults and other problems in wxGTK / wxMSW too i the past.
Anyway, I agree with you that this serious issues should be fixed, I only talking about the schedule and planning.

 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.
 
Ok
I've added the tickets for them just in case.
 
 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.


I hope so :-)
I did some work in advance to evaluate if I was able to make what I originally planned in the proposal, and many of that was direct, like reading the docs and looking for the best matches, most times based on my experience.
Thinks like DC painting and segmentation faults seems to be a different kind and could have ramifications, where I don't have much experience and maybe I don't have the whole picture.
 

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).

 
IIRC merging wxQT into trunk was never a goal of this GSOC project (it is not even mentioned in my proposal, but note that I did the merge of trunk to the wxQT branch already as scheduled).
wxQT for android was a goal stated in my proposal (and even announced in the wxBlog: "Mariano Reingart will continue improving wxQt port and will look into the possibility of bringing wxWidgets to Android via Qt bridge.").
Also wxPython is mentioned explicitly in the deliverables of my proposal. 
I think both could help to interested devs to test or send patches, it takes me a couple of weeks just to get minimal sample compiling and working for wxQT 2.9 in Android (whe I was making my proposal).

I tried write my application as clear as I could, sorry for this misunderstanding.


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.

Done
 
 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.

Ok, that was easy, if you know where to look ;-)

I fixed another in the wxWindow ctor (why it is in a Init() macro in the first place?):


But the DC issues seems to be a bit more complex, I spent some time trying to understand why SetPen calls to ApplyRasterColourOp (and there is no similar function in wxGTK to compare) where there is a uninitialized value  

 
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).


I think I can fix memory leaks, but segv and other DC deeper issues scares me.
I don't think they are fundamental (except for the leaks), as for simple apps they shouldn't arise.
BTW, I consider a simple app = controls sample (so used that to limit the scope)
 
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


No, I didn't, sorry, and it is strange as I looked in the doc folder, but not i contributing
Mental note: do not believe to grep with some keyword to find docs 
 
Do you have any precise questions not addressed there?

Thinks like if I could guard with __WXQT__ (like you say bellow), or what to test, or how to make the unit tests, etc.
Anyway, it is not used in the rest of the code (almost everything is there with a missing implementation where not available already), I'd prefer not to guard WXQT and see the failures / errors


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?


Yes, I'd see the SetParent too.
The problem is that this segv fault is deep on QT's code
 
 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.



Ok 

I've created a new milestone "Unplanned wxQT bug fixes"

https://github.com/reingart/wxWidgets/issues/milestones

If you agree, I set a deadline for July 31, so I have at least 10 days in August to work on "Qt for android" (or not, if at that time you evaluate I should continue fixing wxQT bugs)

Best regards,

Vadim Zeitlin

unread,
Jul 3, 2014, 6:20:34 PM7/3/14
to wx-...@googlegroups.com
On Thu, 3 Jul 2014 16:28:14 -0300 Mariano Reingart wrote:

MR> IIRC merging wxQT into trunk was never a goal of this GSOC project (it is
MR> not even mentioned in my proposal, but note that I did the merge of trunk
MR> to the wxQT branch already as scheduled).

This is a big misunderstanding and we should have made it more explicit,
but the fact is that the goal of absolutely *all* GSoC projects is to be
merged into the trunk at the end of the summer. If this doesn't happen, the
student may pass or fail (both cases have occurred in the past), but from
the point of view of wxWidgets project, absence of a merge == failure.

Realistically, only something available in the trunk, and hence included
in the wxWidgets releases, is really going to be used. And the single most
important thing for any port is to have users (the second one is to have
developers but, and no kidding, having users counts for more).

MR> Also wxPython is mentioned explicitly in the deliverables of my proposal.

Yes, I thought it was overoptimistic to include it then but I wasn't sure
about the actual state of the port and so I thought that perhaps it was
already in an almost mergeable state and that then you could have time to
spend on this. But seeing how things are, this isn't really the case.

MR> But the DC issues seems to be a bit more complex, I spent some time trying
MR> to understand why SetPen calls to ApplyRasterColourOp (and there is no
MR> similar function in wxGTK to compare) where there is a uninitialized value

I have no idea about what does ApplyRasterColourOp() do, but the bug seems
pretty obvious to me: m_rasterColourOp is literally not initialized
anywhere, so you just need to initialize it in wxQtDCImpl ctor, am I
missing something?


[tests]
MR> > Do you have any precise questions not addressed there?
MR>
MR> Thinks like if I could guard with __WXQT__ (like you say bellow), or what
MR> to test, or how to make the unit tests, etc.

Well, the latter is answered in that doc. As for __WXQT__ checks,
obviously the least of them the better but right now you should add as many
of them as needed to make the test suite pass and then remove them as you
implement things/fix bugs.

MR> Anyway, it is not used in the rest of the code (almost everything is there
MR> with a missing implementation where not available already), I'd prefer not
MR> to guard WXQT and see the failures / errors

No, this is a really bad idea. The test suite must pass, like this we can
at least detect the regressions, which often happen due to changes in the
common code in the lesser used ports. If the test suite passed before and
stopped passing after some change, this will be looked into (and hopefully
notified by buildbot). If the test failed with 234 errors before and fails
with 468 errors now, nobody is going to pay the slightest attention to it.

So, please do make sure that the test suite passes.

MR> If you agree, I set a deadline for July 31, so I have at least 10 days in
MR> August to work on "Qt for android" (or not, if at that time you evaluate I
MR> should continue fixing wxQT bugs)

OK, let's do it like this, thanks!
VZ

Mariano Reingart

unread,
Aug 17, 2014, 8:25:16 PM8/17/14
to wx-dev
FYI

I'm working on the documentation, updating and enhancing the wxQT wiki page:



I've also added some install instructions to the docs folder:


It would be great if someone could test the build steps and proof-read the texts, and send me any error, correction or suggestions to improve it.

Questions, comments and advice is welcome

Best regards

Vadim Zeitlin

unread,
Aug 18, 2014, 4:56:35 PM8/18/14
to wx-...@googlegroups.com
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!
VZ

Mariano Reingart

unread,
Aug 18, 2014, 10:30:26 PM8/18/14
to wx-dev
On Mon, Aug 18, 2014 at 5:56 PM, Vadim Zeitlin <va...@wxwidgets.org> wrote:
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).

Ok, all the "Architecture" section or just "internals" subsection?
I think it will make sense moving all that section, as topics are related.
 

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.


Ok, I'll review it after doing the final evaluation for GSOC.
 
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'

I don't understand why they are happening, and even Sean had the same doubt.
Anyway, I will try to review it when I have some time, but if you could be more specific, that would be great to solve it faster.
 
src/common/utilscmn.cpp:1064:56: warning: unused parameter 'flags' [-Wunused-parameter]
 bool wxDoLaunchDefaultBrowser(const wxString& url, int flags)
                                                        ^

This one is not related to Qt (as far I can see), so I tried to not touch things outside the qt subdir.
The following function has a wxUnusedVar but I don't know if that is correct, sorry.
 
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]
 }
 ^

Ok, I'll review the code of Sean as it contributed that part.
At first, it seems harmless as the switch covers the three enum values: Unchecked, PartiallyChecked, Checked
I prefered to left this kind of situations undecided by now instead of adding boilerplate or wrong code.
 
src/qt/cursor.cpp:135:48: warning: unused parameter 'data' [-Wunused-parameter]
 wxCursor::CloneGDIRefData(const wxGDIRefData * data) const
                                                ^

This one I have no idea why Sean commented it out:
Again, I prefer to left this undecided until we have time to review it and do a proper fix.
 
src/generic/graphicc.cpp:1890:73: warning: unused parameter 'window' [-Wunused-parameter]
 wxCairoContext::wxCairoContext( wxGraphicsRenderer* renderer, wxWindow *window)
                                                                         ^

This is due direct m_qtSurface is not supported yet (this needs cairo qt surface)
This should be harmless to, as it should use a pixmap underway (not optimal but working)
 
src/generic/graphicc.cpp:2244:60: warning: unused parameter 'text' [-Wunused-parameter]
 void wxCairoContext::GetPartialTextExtents(const wxString& text, wxArrayDouble& widths) const
                                                            ^

This is not implemented for any other port than wxGTK
I cannot put a WXUNUSED as it will break gtk build (same in the previous one)
 
----------------------------------------------------------------------------

 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.

Ok
 
 There are also things which don't seem to come from wxLogDebug():

----------------------------------------------------------------------------
src/qt/statusbar.cpp(41): Missing implementation of "wxStatusBar::Create parameters"

This is a warning of wxQT, should be harmless to, I didn't have time to remove it as it should be implemented (AFAIK, Sean did some work here too)
 
libGL error: failed to load driver: swrast
QXcbConnection: XCB error: 147 (Unknown), sequence: 162, resource id: 0, major code: 144 (Unknown), minor code: 20

No idea, I never had that error, seems related to opengl?

I did had some strange errors due the log buffer and the console (X), and when the logs output are removed, they should disappear. 
 
----------------------------------------------------------------------------

 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.

I never run this in a remote console, and dialogs / windows allways shows in the default position.
In wxGTK, the about dialog open in the middle of the parent window, should be this the correct position?


 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 :-(


Yes, I noted that too, but seems to happen only if the user closes the window dialog (clicking x) without pressing any button.
Maybe this is related to Qt not destroying the TLW by default:


It is strange as I cannot reproduce this under the debugger:

[Thread 0x7fffdd99e700 (LWP 10650) terminado]
[Thread 0x7fffde9a0700 (LWP 10648) terminado]
[Thread 0x7fffdf818700 (LWP 10647) terminado]
[Thread 0x7fffe0019700 (LWP 10646) terminado]
[Thread 0x7fffea01c700 (LWP 10645) terminado]
[Inferior 1 (process 10644) exited normally]

Maybe it is related to the log output and X console as I commented previously.

No crash related to window destruction is raised on test, so altough it is not perfect, it should work quite stable.
 
 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.


The scroll issue is strange: scroll works if you use the keyboard or the mouse weel, but fails many times if the scrollbar is drag with the mouse.
You can check this with the scroll example.

I've written down this in the wiki, section "Known Open Issues" (drawing and mouse capture)
 
Again, the crash is something sporadic and could be not related to wxQT, the last time I could debug it (using a core file) was something related to the console.
Anyway, if you can send the backtrace to analyze.

Last time we discussed this, you said that there is a tentative fix in the wx trac:


I could not reproduce again under gdb, and the generated core seems to have a problem:

Core was generated by `./drawing'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x00007f1311682729 in ?? ()
(gdb) bt
#0  0x00007f1311682729 in ?? ()
#1  0x0000000000000000 in ?? ()

I hope it will be solved removing the excessive log messages.

 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.


Yes, definitely I wish to continue wxQT after the GSOC, but I need to restart my business and prepare some exams / finish my master thesis, so until January I don't expect having much time to work on this in the short term, but feel free to ask any doubt, I'll try to do my best.

As I said in previous emails, sadly fixing all bugs or complex issues was too extensive (for example, this year other ports had specific GSOC projects to fix unit tests or develop a GDI layer)

I'm glad that all remaining issues were discovered and many core components where implemented this year, so future developers could experiment without so much troubles :-)
 
 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.

Yes, please merge it, it will avoid getting unmaintained as the last attempt (GSOC2010)
It should be updated so no conflict should happen. 
There SHOULD not be non-trivial changes outside qt folders, but please check it as I had to do some big merges (original version was 2.9.2, tree year ago), just in case.
 
 Thanks for your great work!

Thank you, Sean, and all the community !

It has been a nice challenge and a great experience, I could learn a lot and enhance my skills to contribute better to wx in the future.

Best regards,

Vadim Zeitlin

unread,
Aug 19, 2014, 9:12:37 AM8/19/14
to wx-...@googlegroups.com
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.

Thanks again!
VZ

Mariano Reingart

unread,
Aug 24, 2014, 3:41:30 PM8/24/14
to wx-dev
On Tue, Aug 19, 2014 at 10:12 AM, Vadim Zeitlin <va...@wxwidgets.org> wrote:
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.

Ok
 
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.


Did you could 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

 
Ok

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.

I didn't merged changes I disagreed with, see the Pull Request 43:


I just didn't noticed this in the rush, I did a quick review and no unit test failed and not sample stopped to work (AFAIK), so I thought it was harmless (my bad, I'm not  so experienced with wx, will take more care in the future).
 
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__.


Ok, I'll 
 
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.

Ok 
 
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.

Ok, so maybe I had misunderstood your email, I'll review this right from the start
 
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.
 
So sad
 
 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.

Ok, it seems you could made the merge (sorry I couldn't reply before, I have two final exams yesterday and one more next week).

Sadly it seems that svn put all changes in just a commit, losing the history in trunk.

I think that is important, most of the time when I tried to see a file history log to understand some code changes, it just pointed to a previous big commit ("Applied latest wxQt patch from 2010-04-17")

Not only commit messages were lost, also timings (dates) and proper authorship recognition of the changes aren't there (and not only mine, also from other authors too). 

I'll try to commit my changes to GSOC2014_QT just for historic purposes
 
 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.

Ok, I prefer to commit directly to avoid any further issue

Best regards 

Vadim Zeitlin

unread,
Aug 24, 2014, 7:07:44 PM8/24/14
to wx-...@googlegroups.com
On Sun, 24 Aug 2014 16:41:09 -0300 Mariano Reingart wrote:

MR> > MR> > Makefile:20544: warning: overriding recipe for target
MR> > 'monodll_converter.o'
MR> > MR> > Makefile:20541: warning: ignoring old recipe for target
MR> > MR> > 'monodll_converter.o'
MR> > MR> > Makefile:20847: warning: overriding recipe for target
MR> > 'monodll_qt_utils.o'
MR> > MR> > Makefile:20844: warning: ignoring old recipe for target
MR> > MR> > 'monodll_qt_utils.o'
MR> > MR> > Makefile:26406: warning: overriding recipe for target
MR> > 'monolib_converter.o'
MR> > MR> > Makefile:26403: warning: ignoring old recipe for target
MR> > MR> > 'monolib_converter.o'
MR> > MR> > Makefile:26709: warning: overriding recipe for target
MR> > 'monolib_qt_utils.o'
MR> > MR> > Makefile:26706: warning: ignoring old recipe for target
MR> > MR> > 'monolib_qt_utils.o'
MR> > MR> >
MR> > MR>
MR> > MR> I don't understand why they are happening, and even Sean had the same
MR> > doubt.
MR> > MR> Anyway, I will try to review it when I have some time, but if you
MR> > could be
MR> > MR> more specific, that would be great to solve it faster.
MR> >
MR> > I'll look at it.
MR> >
MR> >
MR> Did you could look at it?

No, not yet. I wasted so much time on merging that I really didn't have
any time left for anything else.

MR> Sadly it seems that svn put all changes in just a commit, losing the
MR> history in trunk.

This is just how svn merges work, the problem is that svn:mergeinfo didn't
get filled in. I am still not sure why but it could be because our svn
server was too old and just got upgraded now -- I hoped it would help, but
maybe it had to be upgraded before wxQT branch was created, in which case
it was way too late for this too.

MR> > In any case you shouldn't work in your Github fork any more as the
MR> > branches have diverged now. However you can commit either to svn itself (at
MR> > least to wxQt files) or create new branches for the individual changes on
MR> > Github and submit pull requests that I would then apply. Please let me know
MR> > what do you prefer.
MR>
MR> Ok, I prefer to commit directly to avoid any further issue

Please go ahead. If you have any doubts, please use review.wxwidgets.org
for (more convenient than Github) code reviews.

Also please try to set up a buildbot slave for wxQt if you have a
possibility to do it, this would really help a lot with minimizing breakage
to it.

Thanks!
VZ
Reply all
Reply to author
Forward
0 new messages