[Development] Some QIcon::fromTheme() enhancements

[Simon Hausmann]

On Friday 23. August 2013 10.04.35 Albert Astals Cid wrote:
> On Friday 23 August 2013 09:48:26 Simon Hausmann wrote:
> > 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
> > > 
> > > https://codereview.qt-project.org/#change,63593
> > > approved by you is linked to a P3
> > > 
> > > 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
> > > 
> > > https://codereview.qt-project.org/#change,63413
> > > is linked to a P2
> > > 
> > > https://codereview.qt-project.org/#change,63595
> > > is not linked to any bug
> > > 
> > > 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
> > 
> > Seriously? Three of those are documentation fixes (so P1 or not doesn't
> > make sense). Incorrect device pixel ratio can cause severe misrendering
> > on retina displays, so I'd say it's pretty easy to argue that it should
> > be fixed in patch release. The second last is a build issue that Olivier
> > had nothing to do with and that in Joerg's opinion was important enough
> > to fix in the next patch release. The QString fix you could argue fixes
> > data corruption. And I think similarly it's easy to talk about the pop-up
> > location fix.
> > 
> > Instead of trying to attack other changes or Olivier's judgement as
> > approver, I think it would be much better to explain why it is important
> > that the icon fixes should go into a patch release.
> 
> Honestly, I don't have any stake at the icon fixes, I don't even know if
> they are right, don't assume I'm here because of my @canonical.com address
> i share with Antti.

Ok, I won't make that assumption :)
 
> > Make a good case for
> > the patch, respond with arguments. And if you don't succeed in convincing
> > Olivier, then there are other approvers that might be willing to review
> > this. (I can think of one off the top of my head)
> 
> Please let's not resort to "stop attacking people" as a way to stop people
> disagreeing.
> 
> I'm just wondering why he writes "only P1 for stable" when it's clear this
> rule is not being applied by just reading the first page of
> https://codereview.qt-project.org/#q,status:open+project:qt/qtbase+branch:st
> able,n,z
> 
> I don't mind if he says "These are new features so they need to go into
> dev", but I'm sorry "this is not a P1 so can't go into stable" does not
> sound to me as a valid reason.
> 
> To me something either it is a bug and has to be fixed ASAP or it is a
> feature and can wait. Of course I'm new here so if we have a policy on why
> we delay fixing bugs I'd be happy to be pointed to it.

I think the main reason for being hesitant with fixing a certain bug in stable 
is the risk it introduces to cause other bigger behavioural changes or 
regressions themselves. It is indeed a question of severity, not every bug 
affects the same amount of users, not every bug causes the same amount of 
damage. In Qt we made that mistake a few times as well. Do you remember the 
numerous patch releases in the earlier Qt 4 days? Sometimes we were quite 
relaxed about putting bugfixes into a patch release just to find that it caused 
a regression itself and we needed another patch release as follow up to fix 
regressions in a patch release (urgh). That's one of the reasons I can think 
of why people tend to be a bit more hesitant and not put every bug fix straight 
into a patch release.


Simon