Quantcast

Raiders of the lost patches

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Raiders of the lost patches

Andreas Falkenhahn
I've just upgraded to the latest wxWidgets master but unfortunately, several of my
patches haven't been applied yet... and it's been more than 12 months. Could it be
that those things have simply been forgotten?

The following patches haven't been applied yet although they are over 12 months
old:

1) Patch for wxFontPickerCtrl::SetMaxPointSize() and SetMinPointSize():
http://trac.wxwidgets.org/ticket/17126 

2) GetTopItem() and GetCountPerPage() for wxListBox
http://trac.wxwidgets.org/ticket/17189 

3) GetTopItem() and GetCountPerPage() for wxDataViewCtrl
http://trac.wxwidgets.org/ticket/17498 

4) wxMSW: CanUndo() returns true directly after creation with wxTE_RICH2 flag set
http://trac.wxwidgets.org/ticket/17524 

I know people are probably terribly busy but I think I've already done my
share by fixing bugs myself or implementing things on my own. All I'm asking
for is for someone from the team to pick up on them.

As it is now, it's pretty inconvenient for me because whenever I upgrade to the
latest master my project doesn't build because those patches haven't been applied yet
and I need to disable lots of things until it builds again :/

I think patches should be applied more quickly instead of a few days before
the next release because that won't leave any time to check if there are
any issues with the patches.

Furthermore, if questions concerning those patches arise now I don't remember
anything because it's been over a year. That's another reason why patches
should be checked and applied quicker.

No offense.

--
Best regards,
 Andreas Falkenhahn                          mailto:[hidden email]

--
Please read http://www.wxwidgets.org/support/mlhowto.htm before posting.

To unsubscribe, send email to [hidden email]
or visit http://groups.google.com/group/wx-users
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Raiders of the lost patches

oneeyeman
Andreas,

On Thu, Apr 6, 2017 at 3:41 PM, Andreas Falkenhahn
<[hidden email]> wrote:
> I've just upgraded to the latest wxWidgets master but unfortunately, several of my
> patches haven't been applied yet... and it's been more than 12 months. Could it be
> that those things have simply been forgotten?

It is possible, yes.
It also doesn't hurt to refresh Vadim' memory - it does look like he
is the only one who does
patches and perform the maintenance of the library with little help
from Paul and Vaclav.

But the main and biggest work is done by Vadim.

>
> The following patches haven't been applied yet although they are over 12 months
> old:
>
> 1) Patch for wxFontPickerCtrl::SetMaxPointSize() and SetMinPointSize():
> http://trac.wxwidgets.org/ticket/17126
>
> 2) GetTopItem() and GetCountPerPage() for wxListBox
> http://trac.wxwidgets.org/ticket/17189
>
> 3) GetTopItem() and GetCountPerPage() for wxDataViewCtrl
> http://trac.wxwidgets.org/ticket/17498
>
> 4) wxMSW: CanUndo() returns true directly after creation with wxTE_RICH2 flag set
> http://trac.wxwidgets.org/ticket/17524
>
> I know people are probably terribly busy but I think I've already done my
> share by fixing bugs myself or implementing things on my own. All I'm asking
> for is for someone from the team to pick up on them.
>
> As it is now, it's pretty inconvenient for me because whenever I upgrade to the
> latest master my project doesn't build because those patches haven't been applied yet
> and I need to disable lots of things until it builds again :/
>
> I think patches should be applied more quickly instead of a few days before
> the next release because that won't leave any time to check if there are
> any issues with the patches.
>
> Furthermore, if questions concerning those patches arise now I don't remember
> anything because it's been over a year. That's another reason why patches
> should be checked and applied quicker.
>
> No offense.
>
> --
> Best regards,
>  Andreas Falkenhahn                          mailto:[hidden email]
>
> --
> Please read http://www.wxwidgets.org/support/mlhowto.htm before posting.
>
> To unsubscribe, send email to [hidden email]
> or visit http://groups.google.com/group/wx-users

--
Please read http://www.wxwidgets.org/support/mlhowto.htm before posting.

To unsubscribe, send email to [hidden email]
or visit http://groups.google.com/group/wx-users
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Raiders of the lost patches

Vadim Zeitlin-4
In reply to this post by Andreas Falkenhahn
On Thu, 6 Apr 2017 21:41:02 +0200 Andreas Falkenhahn wrote:

AF> I've just upgraded to the latest wxWidgets master but unfortunately, several of my
AF> patches haven't been applied yet... and it's been more than 12 months. Could it be
AF> that those things have simply been forgotten?

 They haven't been really forgotten, because they are still in the list of
outstanding patches that I religiously keep permanently opened in my
browser, but they did get pushed to the back of the queue... It's not
ideal, to say the least, but in practice patches either get applied
immediately or take a long time, so it's really best to ensure that they
are as simple and as obviously correct as possible to ensure they fall into
the first category. Even more unfortunately, I try to pay more attention to
the new patches than to the updates of the existing ones, so if the first
version has some problems, I might not return to it when it's updated
later.

AF> The following patches haven't been applied yet although they are over
AF> 12 months old:
AF>
AF> 1) Patch for wxFontPickerCtrl::SetMaxPointSize() and SetMinPointSize():
AF> http://trac.wxwidgets.org/ticket/17126 

 This one does look perfectly fine now, so I'm going to apply it, thanks!
Sorry for not re-reviewing it since your update.

AF> 2) GetTopItem() and GetCountPerPage() for wxListBox
AF> http://trac.wxwidgets.org/ticket/17189 

 Here, again, the patch looks mostly fine (although I did make a couple of
minor changes), but the problem was that it was an N-th update to the
ticket so I just missed it, sorry.

 Unfortunately testing it now in the widgets sample (which, conveniently,
already has "Get top item" button in the listbox page), I see that the
result is off by 1 under wxGTK2 as soon as you scroll the listbox so that
the top item is not 0. So this still requires some work... And it would be
great if someone could test it under OS X, please.

AF> 3) GetTopItem() and GetCountPerPage() for wxDataViewCtrl
AF> http://trac.wxwidgets.org/ticket/17498 

 I did actually notice this one but hoped to find time to refactor the code
to avoid duplicating it between wxDataViewCtrl and wxListBox. But since I
didn't manage to do it since a year ago, I probably should just admit that
this is not going to happen, so I resigned to applying it as is.

 However it suffers from the same problem as wxListBox under wxGTK, of
course, so this needs to be fixed before being merged too.


 So while I did push all these patches to Github and created a PR for them
at https://github.com/wxWidgets/wxWidgets/pull/454 to test their
compilation, I can only merge the first one of them into master right now
(and, admittedly, I didn't even test this one neither...).


AF> 4) wxMSW: CanUndo() returns true directly after creation with wxTE_RICH2 flag set
AF> http://trac.wxwidgets.org/ticket/17524 

 This one is entirely my fault, I did see it, but just didn't understand at
all what was going on there. I'll reply in the ticket to keep the
discussion there.

AF> As it is now, it's pretty inconvenient for me because whenever I upgrade to the
AF> latest master my project doesn't build because those patches haven't been applied yet
AF> and I need to disable lots of things until it builds again :/

 The simplest thing is to keep the changes in your own branch and just
merge master into it from time to time. Maintaining your local changes is
relatively painless with Git. But it doesn't mean we shouldn't apply them
quicker, of course...

 Sorry for the delay(s),
VZ

--
TT-Solutions: wxWidgets consultancy and technical support
               http://www.tt-solutions.com/

attachment0 (203 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Raiders of the lost patches

Kevin B. McCarty
On Thursday, April 6, 2017 at 3:03:05 PM UTC-7, Vadim Zeitlin wrote:
On Thu, 6 Apr 2017 21:41:02 +0200 Andreas Falkenhahn wrote:

AF> I've just upgraded to the latest wxWidgets master but unfortunately, several of my
AF> patches haven't been applied yet... and it's been more than 12 months. Could it be
AF> that those things have simply been forgotten?

 They haven't been really forgotten, because they are still in the list of
outstanding patches that I religiously keep permanently opened in my
browser, but they did get pushed to the back of the queue...

If I may piggyback on this thread :-)

Is there something additional that I need to do to get http://trac.wxwidgets.org/ticket/17154 looked at (relating to an unwanted automatic scroll when the user clicks on a widget inside nested wxPanels involving scroll bars)?

I attached both a patch to the samples to demo the issue, and a patch to the library code to fix it ... but no one ever replied, not even to say "Your patch is awful and your bug is invalid" :-) which makes me wonder if I did something wrong in my submission?

Thanks,

--
Kevin B. McCarty
<[hidden email]>

--
Please read http://www.wxwidgets.org/support/mlhowto.htm before posting.
 
To unsubscribe, send email to [hidden email]
or visit http://groups.google.com/group/wx-users
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Raiders of the lost patches

cgodkin
Go Kevin!

carl 

On Apr 7, 2017, at 7:45 AM, [hidden email] wrote:

On Thursday, April 6, 2017 at 3:03:05 PM UTC-7, Vadim Zeitlin wrote:
On Thu, 6 Apr 2017 21:41:02 +0200 Andreas Falkenhahn wrote:

AF> I've just upgraded to the latest wxWidgets master but unfortunately, several of my
AF> patches haven't been applied yet... and it's been more than 12 months. Could it be
AF> that those things have simply been forgotten?

 They haven't been really forgotten, because they are still in the list of
outstanding patches that I religiously keep permanently opened in my
browser, but they did get pushed to the back of the queue...

If I may piggyback on this thread :-)

Is there something additional that I need to do to get http://trac.wxwidgets.org/ticket/17154 looked at (relating to an unwanted automatic scroll when the user clicks on a widget inside nested wxPanels involving scroll bars)?

I attached both a patch to the samples to demo the issue, and a patch to the library code to fix it ... but no one ever replied, not even to say "Your patch is awful and your bug is invalid" :-) which makes me wonder if I did something wrong in my submission?

Thanks,

--
Kevin B. McCarty
<[hidden email]>

--
Please read http://www.wxwidgets.org/support/mlhowto.htm before posting.
 
To unsubscribe, send email to [hidden email]
or visit http://groups.google.com/group/wx-users

--
Please read http://www.wxwidgets.org/support/mlhowto.htm before posting.
 
To unsubscribe, send email to [hidden email]
or visit http://groups.google.com/group/wx-users
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re[2]: Raiders of the lost patches

Vadim Zeitlin-4
In reply to this post by Kevin B. McCarty
On Fri, 7 Apr 2017 07:45:08 -0700 (PDT)  wrote:

> Is there something additional that I need to do to get
> http://trac.wxwidgets.org/ticket/17154 looked at (relating to an unwanted
> automatic scroll when the user clicks on a widget inside nested wxPanels
> involving scroll bars)?
>
> I attached both a patch to the samples to demo the issue, and a patch to
> the library code to fix it ... but no one ever replied, not even to say
> "Your patch is awful and your bug is invalid" :-) which makes me wonder if
> I did something wrong in my submission?

 No, sorry, this one is yet another case... I hoped to find some better
solution for this problem because what we do here is just ugly and it seems
to me there must be some better way to do it. Unfortunately I still didn't
find it (nor even really looked for it, to be honest...) and I guess it's
better to apply this fix because it fixes the behaviour for the users, even
at the cost of making the code even uglier, than do nothing.

 So I just went ahead and applied it. I actually wasn't able to reproduce
the bug though, something seems to be missing in the instructions for doing
it as the window doesn't even have any scrollbars initially. I did resize
it to make one (or both of them) appear, but still couldn't make it scroll
by clicking on the checkbox. Still, I don't think anything has actually
changed in the code to fix this, so the patch is probably still valid and,
again, I applied it in a slightly modified form. Please check I didn't
break anything while replacing your do/while loop with the more usual "for"
one, thanks in advance.

 And please accept my traditional excuses for the delay with dealing with
it,
VZ

--
TT-Solutions: wxWidgets consultancy and technical support
               http://www.tt-solutions.com/

attachment0 (203 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Re[2]: Raiders of the lost patches

Kevin B. McCarty
On Friday, April 7, 2017 at 10:08:05 AM UTC-7, Vadim Zeitlin wrote:
On Fri, 7 Apr 2017 07:45:08 -0700 (PDT)  wrote:

> Is there something additional that I need to do to get
> <a href="http://trac.wxwidgets.org/ticket/17154" target="_blank" rel="nofollow" onmousedown="this.href=&#39;http://www.google.com/url?q\x3dhttp%3A%2F%2Ftrac.wxwidgets.org%2Fticket%2F17154\x26sa\x3dD\x26sntz\x3d1\x26usg\x3dAFQjCNE8DpnLQKecBYRSdrUjMcA_vW6KfA&#39;;return true;" onclick="this.href=&#39;http://www.google.com/url?q\x3dhttp%3A%2F%2Ftrac.wxwidgets.org%2Fticket%2F17154\x26sa\x3dD\x26sntz\x3d1\x26usg\x3dAFQjCNE8DpnLQKecBYRSdrUjMcA_vW6KfA&#39;;return true;">http://trac.wxwidgets.org/ticket/17154 looked at (relating to an unwanted
> automatic scroll when the user clicks on a widget inside nested wxPanels
> involving scroll bars)?

 So I just went ahead and applied it. I actually wasn't able to reproduce
the bug though, something seems to be missing in the instructions for doing
it as the window doesn't even have any scrollbars initially. I did resize
it to make one (or both of them) appear, but still couldn't make it scroll
by clicking on the checkbox.

Hmm, odd.  It may be that something in the layout code has changed since I wrote this patch to the samples, which maybe prevented it from triggering.  I do know that we didn't always see the problem in practice in our real-world app.
 
Still, I don't think anything has actually
changed in the code to fix this, so the patch is probably still valid and,
again, I applied it in a slightly modified form. Please check I didn't
break anything while replacing your do/while loop with the more usual "for"
one, thanks in advance. 

Yes, it looks fine with your revisions.  To be honest I don't remember why I used do/while rather than for.  Possibly because we initially know that 'win' is non-NULL so there is no need to check it on the first loop iteration -- but obviously any performance difference from the additional check is utterly negligible.

Thank you very much for this!

--
Kevin B. McCarty
<[hidden email]>

--
Please read http://www.wxwidgets.org/support/mlhowto.htm before posting.
 
To unsubscribe, send email to [hidden email]
or visit http://groups.google.com/group/wx-users
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Raiders of the lost patches

Andreas Falkenhahn
In reply to this post by Vadim Zeitlin-4
On 07.04.2017 at 00:03 Vadim Zeitlin wrote:

AF>> 2) GetTopItem() and GetCountPerPage() for wxListBox
AF>> http://trac.wxwidgets.org/ticket/17189 

>  Here, again, the patch looks mostly fine (although I did make a couple of
> minor changes), but the problem was that it was an N-th update to the
> ticket so I just missed it, sorry.

>  Unfortunately testing it now in the widgets sample (which, conveniently,
> already has "Get top item" button in the listbox page), I see that the
> result is off by 1 under wxGTK2 as soon as you scroll the listbox so that
> the top item is not 0. So this still requires some work... And it would be
> great if someone could test it under OS X, please.

AF>> 3) GetTopItem() and GetCountPerPage() for wxDataViewCtrl
AF>> http://trac.wxwidgets.org/ticket/17498 

>  I did actually notice this one but hoped to find time to refactor the code
> to avoid duplicating it between wxDataViewCtrl and wxListBox. But since I
> didn't manage to do it since a year ago, I probably should just admit that
> this is not going to happen, so I resigned to applying it as is.

>  However it suffers from the same problem as wxListBox under wxGTK, of
> course, so this needs to be fixed before being merged too.
>  So while I did push all these patches to Github and created a PR for them
> at https://github.com/wxWidgets/wxWidgets/pull/454 to test their
> compilation, I can only merge the first one of them into master right now
> (and, admittedly, I didn't even test this one neither...).

So what should I do next? I'm completely unfamiliar with Git and GitHub
so I don't know what all that means. I've looked at https://github.com/wxWidgets/wxWidgets/pull/454 
but I don't really know what I should do now...

Furthermore, what about the tickets in the Trac? I've seen that on
https://github.com/wxWidgets/wxWidgets/pull/454 you mention that
this closes #17126, #17189 and #17498 but when looking at those
Trac tickets, they haven't been closed yet. Am I supposed to close
them now?

--
Best regards,
 Andreas Falkenhahn                            mailto:[hidden email]

--
Please read http://www.wxwidgets.org/support/mlhowto.htm before posting.

To unsubscribe, send email to [hidden email]
or visit http://groups.google.com/group/wx-users
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re[2]: Raiders of the lost patches

Vadim Zeitlin-4
On Mon, 10 Apr 2017 15:51:35 +0200 Andreas Falkenhahn wrote:

AF> AF>> 3) GetTopItem() and GetCountPerPage() for wxDataViewCtrl
AF> AF>> http://trac.wxwidgets.org/ticket/17498 
AF>
AF> >  I did actually notice this one but hoped to find time to refactor the code
AF> > to avoid duplicating it between wxDataViewCtrl and wxListBox. But since I
AF> > didn't manage to do it since a year ago, I probably should just admit that
AF> > this is not going to happen, so I resigned to applying it as is.
AF>
AF> >  However it suffers from the same problem as wxListBox under wxGTK, of
AF> > course, so this needs to be fixed before being merged too.
AF> >  So while I did push all these patches to Github and created a PR for them
AF> > at https://github.com/wxWidgets/wxWidgets/pull/454 to test their
AF> > compilation, I can only merge the first one of them into master right now
AF> > (and, admittedly, I didn't even test this one neither...).
AF>
AF> So what should I do next?

 Ideally create a patch/ticket/PR to fix the bug, based on the current
state of the branch in the PR 454.

AF> I'm completely unfamiliar with Git and GitHub so I don't know what all
AF> that means.

 Sorry, I can't really explain all this now, so let's just say that you can
get the branch with my changes (i.e. your patches) from the PR 454 and then
you can work on it and make the patches as usual and submit them via Trac
(just attach them to the same #17498, I'll notice them this time). TIA!

AF> Furthermore, what about the tickets in the Trac? I've seen that on
AF> https://github.com/wxWidgets/wxWidgets/pull/454 you mention that
AF> this closes #17126, #17189 and #17498 but when looking at those
AF> Trac tickets, they haven't been closed yet. Am I supposed to close
AF> them now?

 No, they're not closed because this branch hasn't been merged into master
yet. They will be once this happens.

 Regards,
VZ

--
TT-Solutions: wxWidgets consultancy and technical support
               http://www.tt-solutions.com/

attachment0 (188 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Raiders of the lost patches

Andreas Falkenhahn
On 11.04.2017 at 00:03 Vadim Zeitlin wrote:

>  Sorry, I can't really explain all this now, so let's just say that you can
> get the branch with my changes (i.e. your patches) from the PR 454 and then
> you can work on it and make the patches as usual and submit them via Trac
> (just attach them to the same #17498, I'll notice them this time). TIA!

So how can I get a zip or a tarball of the branch in PR 454? I don't see
any download options on github.com.

I've also tried to use git to get the branch from PR 454 by doing the
following:

$ git fetch https://github.com/wxWidgets/wxWidgets.git pull/454/head:wxWidgets-master

But that doesn't work either. I get the following error:

fatal: Not a git repository (or any of the parent directories): .git

--
Best regards,
 Andreas Falkenhahn                            mailto:[hidden email]

--
Please read http://www.wxwidgets.org/support/mlhowto.htm before posting.

To unsubscribe, send email to [hidden email]
or visit http://groups.google.com/group/wx-users
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Raiders of the lost patches

Patrick Geltinger
You have to clone the repository before you can fetch anything:

$ git clone https://github.com/wxWidgets/wxWidgets.git
$ cd wxWidgets
$ git fetch origin pull/454/head:pr-454

That will create a local branch pr-454 which tracks the pull request.

On 12.04.2017 13:36, Andreas Falkenhahn wrote:

> On 11.04.2017 at 00:03 Vadim Zeitlin wrote:
>
>>   Sorry, I can't really explain all this now, so let's just say that you can
>> get the branch with my changes (i.e. your patches) from the PR 454 and then
>> you can work on it and make the patches as usual and submit them via Trac
>> (just attach them to the same #17498, I'll notice them this time). TIA!
> So how can I get a zip or a tarball of the branch in PR 454? I don't see
> any download options on github.com.
>
> I've also tried to use git to get the branch from PR 454 by doing the
> following:
>
> $ git fetch https://github.com/wxWidgets/wxWidgets.git pull/454/head:wxWidgets-master
>
> But that doesn't work either. I get the following error:
>
> fatal: Not a git repository (or any of the parent directories): .git
>

--
Please read http://www.wxwidgets.org/support/mlhowto.htm before posting.

To unsubscribe, send email to [hidden email]
or visit http://groups.google.com/group/wx-users
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Raiders of the lost patches

Andreas Falkenhahn
On 12.04.2017 at 14:02 Patrick Geltinger wrote:

> You have to clone the repository before you can fetch anything:

> $ git clone https://github.com/wxWidgets/wxWidgets.git
> $ cd wxWidgets
> $ git fetch origin pull/454/head:pr-454

> That will create a local branch pr-454 which tracks the pull request.

Thanks, that worked.

I've now examined the issues. I don't really know where to reply because we have
the two Trac tickets #17189 und #17498, we have the PR 454 and we also have
this thread. For simplicity's sake, I'll just reply here.

So Vadim said this:

> Unfortunately testing it now in the widgets sample (which, conveniently,
> already has "Get top item" button in the listbox page), I see that the
> result is off by 1 under wxGTK2 as soon as you scroll the listbox so that
> the top item is not 0. So this still requires some work...

I don't think there's anything we can do here. It's a GTK problem. If you
take a look at the code, you can see that all that wxListBox::GetTopItem()
does is calling gtk_tree_view_get_visible_range() which returns the first
and the last visible item. So it's GTK that's returning items that are off
by 1 here. But, strictly speaking, I think the result is probably correct
and it just looks like it is off by 1 because there's probably just 1 line
of the previous item that is visible. We don't realize this but it's
probably there and that's why gtk_tree_view_get_visible_range() returns
what it does. So I think wxListBox::GetTopItem() is ok because it just
wraps what gtk_tree_view_get_visible_range() does.

If you scroll down to make the top item only partially visible this becomes
even more obvious because then the result is correct and not off by 1.
The off by 1 result is only returned when there's an item which just
*looks* like it is at the top whereas in reality there might be one line
from the previous item above it and that's why you get the off by 1
result.

>  However it suffers from the same problem as wxListBox under wxGTK, of
> course, so this needs to be fixed before being merged too.

See above, same issue, we use gtk_tree_view_get_visible_range() here too
and I think this is definitely the best bet.

--
Best regards,
 Andreas Falkenhahn                            mailto:[hidden email]

--
Please read http://www.wxwidgets.org/support/mlhowto.htm before posting.

To unsubscribe, send email to [hidden email]
or visit http://groups.google.com/group/wx-users
Loading...