[Development] Some QIcon::fromTheme() enhancements

[Olivier Goffart]

On Friday 23 August 2013 09:24:54 Albert Astals Cid wrote:
> On Friday 23 August 2013 07:50:56 Olivier Goffart wrote:
> > On Thursday 22 August 2013 22:18:54 Antti Kaijanmäki wrote:
> > > On 22.08.2013 21:51, Thiago Macieira wrote:
> > > > On quinta-feira, 22 de agosto de 2013 21:26:15, Antti Kaijanmäki 
wrote:
> > > >> I have patches (linked in the bug) to amend this, and I would like to
> > > >> get some feedback on them.
> > > > 
> > > > Hello Antti
> > > > 
> > > > If you can, please upload the patches to codereview.qt-project.org.
> > > > It's
> > > > easier to discuss them there.
> > > 
> > > Either I was not clear enough or I have misunderstood something, but the
> > > links to the patches in codereview.qt-project.org are in that bug
> > > report.
> > > 
> > > :)
> > 
> > Thanks for the patch, I will have a look.
> > However, the patches do not fix a regression, neither a P1 bug, so they
> > should go to the 'dev' branch instead of the 'stable' branch.
> 
> This is weird

I should have perhaps said "critical issue".

Please have a look at the criteria there:
http://qt-project.org/wiki/Branch-Guidelines#4d553711002d5cb36d93954a352efa8e

 
> https://codereview.qt-project.org/#change,63593
> approved by you is linked to a P3

That one is a critical bug.  Reserve should not shrink another QString object. 
Any code that use reserve with a copy of a string is potentially affected.

In my opinion, the bug should have been prioritized P1.

> https://codereview.qt-project.org/#change,63496
> is also linked to a P3
> https://codereview.qt-project.org/#change,63535
> is also linked to a P3

Documentations changes should go in stable

> https://codereview.qt-project.org/#change,63413
> is linked to a P2

I would probably have put this one in dev. This might fit in the bug in new 
feature. Or P2 to get Qt5 mature

> https://codereview.qt-project.org/#change,63595
> is not linked to any bug

Doc change.

> https://codereview.qt-project.org/#change,62408
> is also not linked to any bug
> https://codereview.qt-project.org/#change,62948
> is linked to a bug without categorization


I know nothing about these path, perhaps it should have been in dev, perhaps 
they were good reason to put it there.

In general, I find too many patches goes in the wrong branch.  But that's not a 
justification to put a patch in the wrong branch.

> Also i don't understand that "P1 rule for stable", what's the rationale to
> not fixing bugs in stable? That's what stable branch is for, no?

Stable means it does not change much.

Each change, no matter how small, has the potential to break things.
In order to ensure that Qt x.y.(z+1) is better than Qt x.y.z,  we need to have 
some sort of restrictions on what changes applies.

> I could understand "P1 rule for release", but for stable, what if i fix a
> small bug (which I understand won't be categorized as P1), why should people
> wait for the fix of that small bug for Qt 5.2 instead of 5.1.x?

The bug was already there since the beginning of the feature in Qt 4.7.  They 
already had to wait so many years, they can wait a few moth more.

However, a regression in Qt 5.1.2 would be much more damageable.  Maybe there 
is a broken small .xpm in one style that makes an otherwise good png in 
another theme not taken. Maybe this happens to be a huge performance 
bottleneck with some setup, maybe there is a bug in XPM parsing. Or any other 
problem we don't think about now.


You would have to have good arguments that this bug is urgent enough. 
Following a specification is important of course, but it is only some details 
about it and it does not seem an urgent issue to me.

-- 
Olivier

Woboq - Qt services and support - http://woboq.com - http://code.woboq.org