[Development] Some QIcon::fromTheme() enhancements

[Albert Astals Cid]

On Friday 23 August 2013 10:24:01 Simon Hausmann wrote:
> 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.

"Sorry but this touches too much inner stuff, let's delay it a bit so we have 
more time/people to test/review it".

That's also a very valid reason for me to put stuff into dev.

I was only saying that I found the "only P1 in stable" reason a not so good 
reason given the track record of stuff that ends up in stable.

Cheers,
  Albert

> 
> 
> Simon