- User Since
- May 6 2017, 7:59 PM (36 w, 5 d)
@davidedmundson Could you confirm/refute Anthony's concern?
Great work, thanks Peter!
Looked at the patch, I think it solves the bug in the best way possible. Perfect commit message, test plan and code.
@anthonyfieroni The "perfect forward" and the RAII semantics of DialogHelper are quite neat (a comment to make it easier to figure out the purpose could be added to the class, though, as Albert hints at). Still, I think Elvis also makes a good point, and I might prefer his solution actually.
Won't comment on the patch, I just have some tips regarding future summary / commit messages in case you didn't know those already :). Have a look at https://chris.beams.io/posts/git-commit/, especially 3. and 5.
Thanks for the Diff. I cannot try your patch on Windows, but if it works for you and fixes the problem I guess this is fine.
@rempt Hi Boud, greetings from Gwenview land. I know you are busy, but it would be great if you could help us out with a question we have regarding an old commit of yours to Gwenview from 5 years ago: In d5635328f33c you introduce some colour correction and in particular INTENT_PERCEPTUAL, cmsFLAGS_BLACKPOINTCOMPENSATION.
First things first: The patch marks an impressive improvement in (perceived) drawing performance for slow-rendering PDFs, I'm glad your hard work payed off.
Mon, Jan 15
Read the linked blog, looked at the code and tested whether the affected dialogs in Gwenview still work. LGTM (apart from the location of dialoghelper.h), but I'd like to get a second opinion from someone longer in the game.
Wed, Jan 10
Accepting for now, but before committing, let's wait how D9731 plays out. Due to the i18n changes your patch is headed for master / 18.04 anyway.
Another thought I had: The thumbnail slider in Browse mode has a tooltip displaying the thumbnail size in pixels. Suppose the user moves the window from a low-res display (set to 1x scaling) to a HiDPI display (set to 2x scaling). Obviously, the size of the thumbnails (as measured with a tape measure) should stay the same, meaning the pixmap used to achieve this must become larger.
Tue, Jan 9
Thanks for submitting this.
Mon, Jan 8
Finally got a chance to try your latest changes, here's how it looks for me:
Sun, Jan 7
@muhlenpfordt I promised to fix this for 17.12.1, but as this would need to go in by tomorrow and is pretty trivial, I already committed this. Still I'd appreciate it if you could have a short look, i.e. "audit" the commit – Thanks!
I think so. Thanks for all the tweaks, BTW.
What is Normal Mode to the user, anyway? It's not bad, but does not click with me immediately, TBH.
Sat, Jan 6
@aacid Are you okay with this change?
That's even better. Just update the Diff, I'll check / approve and then we'll see what Albert says.
Sounds great. Maybe "View Mode" in the title? ("Mode" is not really meaningful in itself.)
Thanks for the review and your help ;)
(Helps saving inline comments before pressing "Submit").
Yep. Take your time with rephrasing, it's 18.04 material anyway. Also, it should probably stay being a question (because it's not a warning or information either), hence the question mark icon.
Now we have "Presentation Mode" (title), "Presentation Mode" (first sentence), "Presentation Mode" (second sentence), "Presentation Mode" (button). I find it a bit annoying to read ;) Can we do something about it, e.g. removing the second sentence? Any other idea?
I saw other functions in this file are written as is but they are wrong too.
Seems fine functionality-wise and makes for a nice improvement in usability.
You should open a 48h bugfixing service ;)
Thanks for the update. I won't need to test again, as the screenshots clearly show it working fine ;)
Fri, Jan 5
In Phabricator's side bar there is Edit File, try if this works for you.
Thu, Jan 4
Wed, Jan 3
Thanks for the video, you are making good progress towards changing things for the better. There are two root problems I can extract from it:
Happy new year, everyone ;)
Essentially this: D9549#184975 (but a "No" option could be added).
Phabricator has an application called "Ponder" (not available on our instance yet) which essentially supports Q&A style workflows. This would nicely integrate with the existing notifications and project tags, and every forum user already has an Identity account anyway.
Thanks for the patch.
Thanks Nate, your blog readership seems to like it too (congratulations on the blog, BTW ;) What about the inevitable "But can I change it back?". You claim we allow this, but if you actually try it, it does not work, i.e. the menu shadow is now almost completely gone for the former default shadow size of 16px. This is also the case for any user upgrading to 5.12 with a non-default size, e.g. 17px, where the shadow going away looks like an upgrade bug.
Tue, Jan 2
In those links, I don't see any mention of changes to identity (which is different from Phab, right!?) as I am proposing, but I'll stop here. We'll need to wait for @bcooksley to comment, maybe he knows more about the current upstream status of the issue.
Of course I did read the entire ticket before posting. Care to elaborate what point I am missing? Note I am talking about identity (i.e. something we can fix now), while the proper fix would be in Phabricator (i.e. something which might be very far away).
I agree in light of our newly selected focus goals, i.e. T7116: Streamlined onboarding of new contributors, that this is kind of important to solve.
Now I am confused. I thought the screenshots above show what the VDG proposed, i.e. small menu shadows, large window shadows? At least I like it this way.
Mon, Jan 1
New year, new look ;) Great work, this looks impressive and solves the left-side usability problem at the same time.
Sun, Dec 31
Sat, Dec 30
Tue, Dec 26
I agree there is still room for improvements to make it easier to register. People I have in mind here are those who want to start contributing changes to the code, get involved in the VDG, do promotional things etc., i.e. everything not related to bug reporting or triaging for which Bugzilla is the place to go at the moment.
Sat, Dec 23
@behrmann Thanks so much, your insights and perspective are really helpful.
Fri, Dec 22
Thu, Dec 21
There are still a couple of issues, so I guess it might take some time until this patch can land. Note I have neither tested all things I want to test yet nor done a full code review. I'll do those once more things work.
Wed, Dec 20
Works now as it should, but I'm still not feeling 100% confident (but a lot better than with the previous iteration :)
Dec 19 2017
Dec 18 2017
Perfect. Please commit to Applications/17.12, I'll do the follow-up fix and merge to master later.
Okay, played around a bit and thought about our options. In general it works good enough for me in normal and (since the latest Diff) in fullscreen mode and for opaque / non-opaque as well as for fully white / fully black images.
Dec 17 2017
Dec 16 2017
Dec 15 2017
Thanks so much for your help, Renato! Seems like a straightforward fix for someone familiar with the new additions to KFilePlacesModel ;)