Re: Review Request 109945: [Screenlocker] Clear password field on Escape keypress
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109945/#review30903 --- Ship it! Ship It! - Aaron J. Seigo On April 10, 2013, 8:08 p.m., Dan Vrátil wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/109945/ > --- > > (Updated April 10, 2013, 8:08 p.m.) > > > Review request for kde-workspace. > > > Description > --- > > When you type a password in the locker and press escape, the password > disappears. Now when you press any key, the password reappears and the > pressed key is appended. > > This patch forces the password input to be cleaned on escape keypress. > > > Diffs > - > > ksmserver/screenlocker/greeter/greeter.h a0a1ac3 > ksmserver/screenlocker/greeter/greeter.cpp f5918f9 > > ksmserver/screenlocker/greeter/themes/org.kde.passworddialog/contents/ui/Greeter.qml > 3dd9fc0 > > Diff: http://git.reviewboard.kde.org/r/109945/diff/ > > > Testing > --- > > > Thanks, > > Dan Vrátil > >
Re: Review Request 109945: [Screenlocker] Clear password field on Escape keypress
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109945/#review30910 --- This review has been submitted with commit 06d67a3a0ca6c5e9ea545b40d04a300f316b9405 by Dan Vrátil to branch KDE/4.10. - Commit Hook On April 10, 2013, 8:08 p.m., Dan Vrátil wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/109945/ > --- > > (Updated April 10, 2013, 8:08 p.m.) > > > Review request for kde-workspace. > > > Description > --- > > When you type a password in the locker and press escape, the password > disappears. Now when you press any key, the password reappears and the > pressed key is appended. > > This patch forces the password input to be cleaned on escape keypress. > > > Diffs > - > > ksmserver/screenlocker/greeter/greeter.h a0a1ac3 > ksmserver/screenlocker/greeter/greeter.cpp f5918f9 > > ksmserver/screenlocker/greeter/themes/org.kde.passworddialog/contents/ui/Greeter.qml > 3dd9fc0 > > Diff: http://git.reviewboard.kde.org/r/109945/diff/ > > > Testing > --- > > > Thanks, > > Dan Vrátil > >
Re: Review Request 109945: [Screenlocker] Clear password field on Escape keypress
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109945/ --- (Updated April 11, 2013, 11:31 a.m.) Status -- This change has been marked as submitted. Review request for kde-workspace. Description --- When you type a password in the locker and press escape, the password disappears. Now when you press any key, the password reappears and the pressed key is appended. This patch forces the password input to be cleaned on escape keypress. Diffs - ksmserver/screenlocker/greeter/greeter.h a0a1ac3 ksmserver/screenlocker/greeter/greeter.cpp f5918f9 ksmserver/screenlocker/greeter/themes/org.kde.passworddialog/contents/ui/Greeter.qml 3dd9fc0 Diff: http://git.reviewboard.kde.org/r/109945/diff/ Testing --- Thanks, Dan Vrátil
Re: Review Request 109935: Make KFilePlacesModel treats internal moves by drag'n'drop as moves instead of remove+insert
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109935/#review30912 --- This review has been submitted with commit 8f8e3c27005b9d32b31cb6de5fd162a68c2a736b by Aurélien Gâteau to branch master. - Commit Hook On April 9, 2013, 3:56 p.m., Aurélien Gâteau wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/109935/ > --- > > (Updated April 9, 2013, 3:56 p.m.) > > > Review request for kdelibs and Kevin Ottens. > > > Description > --- > > When user moves an item from the model by dragging it, KFilePlacesModel > removes the item row and adds it back at the new position. This patch changes > this behavior to use beginMoveRows() and endMoveRows() instead. > > I came upon this while implementing drag'n'drop to reorder items in Homerun. > Homerun uses KFilePlacesModel in the "favorite places" source, and the > reordering code expects the model to use beginMoveRows() and endMoveRows(). > > > Diffs > - > > kfile/kfileplacesmodel.cpp 0192926233bac62596e4c04b9267229f4a284c95 > kfile/tests/kfileplacesmodeltest.cpp > 2ae9b7548e47985369dc896086031c07bf0c5a4c > > Diff: http://git.reviewboard.kde.org/r/109935/diff/ > > > Testing > --- > > Tested with Homerun and other applications using KFileDialog. Updated the > unit-tests. > > > Thanks, > > Aurélien Gâteau > >
Re: Review Request 109935: Make KFilePlacesModel treats internal moves by drag'n'drop as moves instead of remove+insert
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109935/ --- (Updated April 11, 2013, 12:03 p.m.) Status -- This change has been marked as submitted. Review request for kdelibs and Kevin Ottens. Description --- When user moves an item from the model by dragging it, KFilePlacesModel removes the item row and adds it back at the new position. This patch changes this behavior to use beginMoveRows() and endMoveRows() instead. I came upon this while implementing drag'n'drop to reorder items in Homerun. Homerun uses KFilePlacesModel in the "favorite places" source, and the reordering code expects the model to use beginMoveRows() and endMoveRows(). Diffs - kfile/kfileplacesmodel.cpp 0192926233bac62596e4c04b9267229f4a284c95 kfile/tests/kfileplacesmodeltest.cpp 2ae9b7548e47985369dc896086031c07bf0c5a4c Diff: http://git.reviewboard.kde.org/r/109935/diff/ Testing --- Tested with Homerun and other applications using KFileDialog. Updated the unit-tests. Thanks, Aurélien Gâteau
Re: Review Request 109935: Make KFilePlacesModel treats internal moves by drag'n'drop as moves instead of remove+insert
> On April 11, 2013, 2:03 p.m., Commit Hook wrote: > > This review has been submitted with commit > > 8f8e3c27005b9d32b31cb6de5fd162a68c2a736b by Aurélien Gâteau to branch > > master. I decided to push it to master and not to KDE/4.10 as this could be disruptive. - Aurélien --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109935/#review30912 --- On April 11, 2013, 2:03 p.m., Aurélien Gâteau wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/109935/ > --- > > (Updated April 11, 2013, 2:03 p.m.) > > > Review request for kdelibs and Kevin Ottens. > > > Description > --- > > When user moves an item from the model by dragging it, KFilePlacesModel > removes the item row and adds it back at the new position. This patch changes > this behavior to use beginMoveRows() and endMoveRows() instead. > > I came upon this while implementing drag'n'drop to reorder items in Homerun. > Homerun uses KFilePlacesModel in the "favorite places" source, and the > reordering code expects the model to use beginMoveRows() and endMoveRows(). > > > Diffs > - > > kfile/kfileplacesmodel.cpp 0192926233bac62596e4c04b9267229f4a284c95 > kfile/tests/kfileplacesmodeltest.cpp > 2ae9b7548e47985369dc896086031c07bf0c5a4c > > Diff: http://git.reviewboard.kde.org/r/109935/diff/ > > > Testing > --- > > Tested with Homerun and other applications using KFileDialog. Updated the > unit-tests. > > > Thanks, > > Aurélien Gâteau > >
Re: Review Request 106581: Make KFileDialog remember settings
> On April 11, 2013, 12:52 a.m., Albert Astals Cid wrote: > > Aurelien: David: apaku: Ping? Still in my TODO list :/ - Aurélien --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106581/#review30891 --- On Oct. 2, 2012, 12:55 p.m., Aurélien Gâteau wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/106581/ > --- > > (Updated Oct. 2, 2012, 12:55 p.m.) > > > Review request for kdelibs and Andreas Pakulat. > > > Description > --- > > This patch makes KFileDialog remember settings such as which view mode is > selected and whether the places sidebar should be visible. > > Original code tried to save those to kdeglobals so that changes would be > shared among all applications but it did so the wrong way. The patch writes > the configuration to kdeglobals correctly, but saves the KDirOperator to the > application config file (KDirOperator configuration settings are sort > settings, show preview, show hidden files, view style (icon, detail, > treeview)) > > There are two reasons for not saving KDirOperator config to kdeglobals: > > 1. It is right now not possible to tell KDirOperator::writeConfig() to save > to kdeglobals. It could be done by adding a new version of writeConfig() > which would accept a KConfigBase::WriteFlags argument though. > > 2. It probably would not be a good idea to remember KDirOperator settings > globally anyway because depending on the application one may want to use > different settings. > For example if user wants to select images or videos he might set the file > dialog to show big icons and the preview pane (so that videos can be played). > This setup would however not be adapted in an application where one wants to > select a text file. > > > This addresses bug 139475. > http://bugs.kde.org/show_bug.cgi?id=139475 > > > Diffs > - > > kfile/kfilewidget.cpp 8e2f967 > > Diff: http://git.reviewboard.kde.org/r/106581/diff/ > > > Testing > --- > > Tested with two different KDE applications. Settings are correctly remembered. > > > Thanks, > > Aurélien Gâteau > >
Re: Review Request 109792: Update 'dim display' algorithm.
> On March 31, 2013, 4:14 p.m., Àlex Fiestas wrote: > > To be honest I don't like adding yet another configuration option by > > default, a configuration option that apparently is needed only in some > > systems. > > > > There is no other alternative than this? > > Danny Baumann wrote: > Well, I don't see any. I tried to approach this at the kernel level > first, but the kernel people rejected the idea. But I don't see it as a big > problem either: E.g. Windows (which isn't an example of overconfigurability) > has a dimmed brightness setting as well. While it's required for my laptop, I > think it can be useful for other people as well. When I hit the problem, at > first I couldn't believe this isn't configurable either ;) > > BTW, one thing I'm not yet certain about is whether the option should be > 'dim to' or 'dim by'. Currently it's the former, but the latter might it make > more obvious that the value is relative to the non-dimmed brightness. > > Sebastian Kügler wrote: > Just set the minimum brightness to 1 instead of 0. We're generally not > adding config options for this kind of things. > > Danny Baumann wrote: > That doesn't help though, as with intel-backlight 1 is still way too low > to be visible. I think the value corresponding to acpi value 0 is about 5%. > At this point, I think, I would alter the acpi behaviour again. Also, if the > user had set the brightness to 0 before for some reason (e.g. because he's > working in the evening), 'dim display' would suddenly make the display more > bright. > In general, the problem is the different backlight interfaces just behave > differently, which is why I think this problem can't be solved with a > hardcoded value as long as it's not possible to detect the used sysfs file > (which isn't the case as long as the used X driver provides a backlight > interface via Xrandr). > > > Sebastian Kügler wrote: > The "dim can brighten up" is a different problem that needs a different > solution, it's unrelated here. > > I don't see how a config option solves this problem. Even with a config > option, we still don't know what the default should be. Also, users are not > going to look for such an option (which maybe says enough about the option > itself). Users are going to hit the brightness slider when the brightness is > too low, so 1 actually does make sense. > > Danny Baumann wrote: > A sane value for the default would be 'retain current behaviour' (that > is: dim by 100%). > Is there evidence for the 'users are not going to look for such an > option' assertion? I know for a fact that I _did_ search for that option > (until I looked up in the source that it's nowhere to be found). Surely this > one sample doesn't represent the KDE user base - but do you have any > representative data, especially given that the KDE user base in general > doesn't have a problem with a slightly-higher-than-usual amount of settings > (which is also evident in the very dialog we're talking about: are there that > many users who look for e.g. a highly configurable 'run script on power > status change' option?) > > I'm also not sure what you mean by 'Users are going to hit the brightness > slider when the brightness is too low', as we're talking about the > dimmed-down case after all, where the user was idle for the configured amount > of time. But as I wrote above, neither 0 nor 1 works for any user who is not > using the acpi_video backlight controls for whatever reason (e.g. because > it's buggy, or simply not present at all). I'm open for suggestions on how to > fix this problem without config option (even if I think the option makes > sense), but just raising the hardcoded value from 0 to 1 definitely does not > help. > > Sebastian Kügler wrote: > What would probably make sense it to use 10% or 1 (whichever is more) as > lowest value. We do know the max value, so we can make sure it's low, but not > zero. This should be tried. > > How does raising it from 0 to 1 not help, btw? > > Danny Baumann wrote: > Raising to 10% would help in my case, but how do you suggest handling > cases where either > - 10% is considered 'too bright' (given that it was darker before - > people might complain about 'more power usage than necessary') and > - 10% is brighter than the brightness before dimming? > > The answer to 'why does raising to 1 not help' is a few comments above ;) > "That doesn't help though, as with intel-backlight 1 is still way too low > to be visible. I think the value corresponding to acpi value 0 is about 5%" > > But can you please explain where exactly you see the problem of this > option compared to e.g. the way more sophisticated 'run script' options? > > Sebastian Kügler wrote: > The option does not solve the problem. We have to make sure the system is > usable without changing an option. qMax(5%, 1) is also f
Re: Review Request 109792: Update 'dim display' algorithm.
> On March 31, 2013, 4:14 p.m., Àlex Fiestas wrote: > > To be honest I don't like adding yet another configuration option by > > default, a configuration option that apparently is needed only in some > > systems. > > > > There is no other alternative than this? > > Danny Baumann wrote: > Well, I don't see any. I tried to approach this at the kernel level > first, but the kernel people rejected the idea. But I don't see it as a big > problem either: E.g. Windows (which isn't an example of overconfigurability) > has a dimmed brightness setting as well. While it's required for my laptop, I > think it can be useful for other people as well. When I hit the problem, at > first I couldn't believe this isn't configurable either ;) > > BTW, one thing I'm not yet certain about is whether the option should be > 'dim to' or 'dim by'. Currently it's the former, but the latter might it make > more obvious that the value is relative to the non-dimmed brightness. > > Sebastian Kügler wrote: > Just set the minimum brightness to 1 instead of 0. We're generally not > adding config options for this kind of things. > > Danny Baumann wrote: > That doesn't help though, as with intel-backlight 1 is still way too low > to be visible. I think the value corresponding to acpi value 0 is about 5%. > At this point, I think, I would alter the acpi behaviour again. Also, if the > user had set the brightness to 0 before for some reason (e.g. because he's > working in the evening), 'dim display' would suddenly make the display more > bright. > In general, the problem is the different backlight interfaces just behave > differently, which is why I think this problem can't be solved with a > hardcoded value as long as it's not possible to detect the used sysfs file > (which isn't the case as long as the used X driver provides a backlight > interface via Xrandr). > > > Sebastian Kügler wrote: > The "dim can brighten up" is a different problem that needs a different > solution, it's unrelated here. > > I don't see how a config option solves this problem. Even with a config > option, we still don't know what the default should be. Also, users are not > going to look for such an option (which maybe says enough about the option > itself). Users are going to hit the brightness slider when the brightness is > too low, so 1 actually does make sense. > > Danny Baumann wrote: > A sane value for the default would be 'retain current behaviour' (that > is: dim by 100%). > Is there evidence for the 'users are not going to look for such an > option' assertion? I know for a fact that I _did_ search for that option > (until I looked up in the source that it's nowhere to be found). Surely this > one sample doesn't represent the KDE user base - but do you have any > representative data, especially given that the KDE user base in general > doesn't have a problem with a slightly-higher-than-usual amount of settings > (which is also evident in the very dialog we're talking about: are there that > many users who look for e.g. a highly configurable 'run script on power > status change' option?) > > I'm also not sure what you mean by 'Users are going to hit the brightness > slider when the brightness is too low', as we're talking about the > dimmed-down case after all, where the user was idle for the configured amount > of time. But as I wrote above, neither 0 nor 1 works for any user who is not > using the acpi_video backlight controls for whatever reason (e.g. because > it's buggy, or simply not present at all). I'm open for suggestions on how to > fix this problem without config option (even if I think the option makes > sense), but just raising the hardcoded value from 0 to 1 definitely does not > help. > > Sebastian Kügler wrote: > What would probably make sense it to use 10% or 1 (whichever is more) as > lowest value. We do know the max value, so we can make sure it's low, but not > zero. This should be tried. > > How does raising it from 0 to 1 not help, btw? > > Danny Baumann wrote: > Raising to 10% would help in my case, but how do you suggest handling > cases where either > - 10% is considered 'too bright' (given that it was darker before - > people might complain about 'more power usage than necessary') and > - 10% is brighter than the brightness before dimming? > > The answer to 'why does raising to 1 not help' is a few comments above ;) > "That doesn't help though, as with intel-backlight 1 is still way too low > to be visible. I think the value corresponding to acpi value 0 is about 5%" > > But can you please explain where exactly you see the problem of this > option compared to e.g. the way more sophisticated 'run script' options? > > Sebastian Kügler wrote: > The option does not solve the problem. We have to make sure the system is > usable without changing an option. qMax(5%, 1) is also f
Re: Review Request 109792: Update 'dim display' algorithm.
On Thursday, April 11, 2013 15:11:54 Danny Baumann wrote: > Having said that, my current plan is the following: > - Drop the UI option > - However, leave the configurability via kconfig intact. That way, users who > are not happy with the defaults do at least have a remote chance of > changing it without recompiling KDE (after they found it's configurable, of > course - but there's a huge step between looking something up and compiling > a whole DE to change it) - Additionally add a dimRelative kconfig setting. > With dimRelative=true, dimRatio is interpreted as a factor > (dimmedBrightness = origBrightness * dimRatio), with it being false it's > interpreted as an absolute value (dimmedBrightness = 100f * dimRatio). - > The defaults are set up as dimRatio = 0.05, dimRelative = false > - Do the dimming in 2 steps, the first one being done after the configured > time, the second one 10 seconds later > > Is that approach ok? Nope, even a config key just adds maintainability and support burden. I've been there, done that, and didn't like the ride. The bug you are addressing is entirely fixable without an option, there is simply not a good reason to make this configurable. Cheers, -- sebas http://www.kde.org | http://vizZzion.org | GPG Key ID: 9119 0EF9
Re: Review Request 109792: Update 'dim display' algorithm.
> On March 31, 2013, 4:14 p.m., Àlex Fiestas wrote: > > To be honest I don't like adding yet another configuration option by > > default, a configuration option that apparently is needed only in some > > systems. > > > > There is no other alternative than this? > > Danny Baumann wrote: > Well, I don't see any. I tried to approach this at the kernel level > first, but the kernel people rejected the idea. But I don't see it as a big > problem either: E.g. Windows (which isn't an example of overconfigurability) > has a dimmed brightness setting as well. While it's required for my laptop, I > think it can be useful for other people as well. When I hit the problem, at > first I couldn't believe this isn't configurable either ;) > > BTW, one thing I'm not yet certain about is whether the option should be > 'dim to' or 'dim by'. Currently it's the former, but the latter might it make > more obvious that the value is relative to the non-dimmed brightness. > > Sebastian Kügler wrote: > Just set the minimum brightness to 1 instead of 0. We're generally not > adding config options for this kind of things. > > Danny Baumann wrote: > That doesn't help though, as with intel-backlight 1 is still way too low > to be visible. I think the value corresponding to acpi value 0 is about 5%. > At this point, I think, I would alter the acpi behaviour again. Also, if the > user had set the brightness to 0 before for some reason (e.g. because he's > working in the evening), 'dim display' would suddenly make the display more > bright. > In general, the problem is the different backlight interfaces just behave > differently, which is why I think this problem can't be solved with a > hardcoded value as long as it's not possible to detect the used sysfs file > (which isn't the case as long as the used X driver provides a backlight > interface via Xrandr). > > > Sebastian Kügler wrote: > The "dim can brighten up" is a different problem that needs a different > solution, it's unrelated here. > > I don't see how a config option solves this problem. Even with a config > option, we still don't know what the default should be. Also, users are not > going to look for such an option (which maybe says enough about the option > itself). Users are going to hit the brightness slider when the brightness is > too low, so 1 actually does make sense. > > Danny Baumann wrote: > A sane value for the default would be 'retain current behaviour' (that > is: dim by 100%). > Is there evidence for the 'users are not going to look for such an > option' assertion? I know for a fact that I _did_ search for that option > (until I looked up in the source that it's nowhere to be found). Surely this > one sample doesn't represent the KDE user base - but do you have any > representative data, especially given that the KDE user base in general > doesn't have a problem with a slightly-higher-than-usual amount of settings > (which is also evident in the very dialog we're talking about: are there that > many users who look for e.g. a highly configurable 'run script on power > status change' option?) > > I'm also not sure what you mean by 'Users are going to hit the brightness > slider when the brightness is too low', as we're talking about the > dimmed-down case after all, where the user was idle for the configured amount > of time. But as I wrote above, neither 0 nor 1 works for any user who is not > using the acpi_video backlight controls for whatever reason (e.g. because > it's buggy, or simply not present at all). I'm open for suggestions on how to > fix this problem without config option (even if I think the option makes > sense), but just raising the hardcoded value from 0 to 1 definitely does not > help. > > Sebastian Kügler wrote: > What would probably make sense it to use 10% or 1 (whichever is more) as > lowest value. We do know the max value, so we can make sure it's low, but not > zero. This should be tried. > > How does raising it from 0 to 1 not help, btw? > > Danny Baumann wrote: > Raising to 10% would help in my case, but how do you suggest handling > cases where either > - 10% is considered 'too bright' (given that it was darker before - > people might complain about 'more power usage than necessary') and > - 10% is brighter than the brightness before dimming? > > The answer to 'why does raising to 1 not help' is a few comments above ;) > "That doesn't help though, as with intel-backlight 1 is still way too low > to be visible. I think the value corresponding to acpi value 0 is about 5%" > > But can you please explain where exactly you see the problem of this > option compared to e.g. the way more sophisticated 'run script' options? > > Sebastian Kügler wrote: > The option does not solve the problem. We have to make sure the system is > usable without changing an option. qMax(5%, 1) is also f
Re: Parsing a user-entered localized datetime
Hi Denis, On Wednesday, 2013-04-10, Denis Steckelmacher wrote: > to other calendar systems. Does KDE store its locale-specific settings > in files > that can be easily edited by translators ? Can't help you with the rest, but one example of locale specific settings are (street-)address formatting rules. You can find those files in kde-runtime/l10n, lool for config key AddressFormat in the entry.desktop files for various locales. Cheers, Kevin -- Kevin Krammer, KDE developer, xdg-utils developer KDE user support, developer mentoring signature.asc Description: This is a digitally signed message part.
Re: Review Request 109792: Update 'dim display' algorithm.
> On March 31, 2013, 4:14 p.m., Àlex Fiestas wrote: > > To be honest I don't like adding yet another configuration option by > > default, a configuration option that apparently is needed only in some > > systems. > > > > There is no other alternative than this? > > Danny Baumann wrote: > Well, I don't see any. I tried to approach this at the kernel level > first, but the kernel people rejected the idea. But I don't see it as a big > problem either: E.g. Windows (which isn't an example of overconfigurability) > has a dimmed brightness setting as well. While it's required for my laptop, I > think it can be useful for other people as well. When I hit the problem, at > first I couldn't believe this isn't configurable either ;) > > BTW, one thing I'm not yet certain about is whether the option should be > 'dim to' or 'dim by'. Currently it's the former, but the latter might it make > more obvious that the value is relative to the non-dimmed brightness. > > Sebastian Kügler wrote: > Just set the minimum brightness to 1 instead of 0. We're generally not > adding config options for this kind of things. > > Danny Baumann wrote: > That doesn't help though, as with intel-backlight 1 is still way too low > to be visible. I think the value corresponding to acpi value 0 is about 5%. > At this point, I think, I would alter the acpi behaviour again. Also, if the > user had set the brightness to 0 before for some reason (e.g. because he's > working in the evening), 'dim display' would suddenly make the display more > bright. > In general, the problem is the different backlight interfaces just behave > differently, which is why I think this problem can't be solved with a > hardcoded value as long as it's not possible to detect the used sysfs file > (which isn't the case as long as the used X driver provides a backlight > interface via Xrandr). > > > Sebastian Kügler wrote: > The "dim can brighten up" is a different problem that needs a different > solution, it's unrelated here. > > I don't see how a config option solves this problem. Even with a config > option, we still don't know what the default should be. Also, users are not > going to look for such an option (which maybe says enough about the option > itself). Users are going to hit the brightness slider when the brightness is > too low, so 1 actually does make sense. > > Danny Baumann wrote: > A sane value for the default would be 'retain current behaviour' (that > is: dim by 100%). > Is there evidence for the 'users are not going to look for such an > option' assertion? I know for a fact that I _did_ search for that option > (until I looked up in the source that it's nowhere to be found). Surely this > one sample doesn't represent the KDE user base - but do you have any > representative data, especially given that the KDE user base in general > doesn't have a problem with a slightly-higher-than-usual amount of settings > (which is also evident in the very dialog we're talking about: are there that > many users who look for e.g. a highly configurable 'run script on power > status change' option?) > > I'm also not sure what you mean by 'Users are going to hit the brightness > slider when the brightness is too low', as we're talking about the > dimmed-down case after all, where the user was idle for the configured amount > of time. But as I wrote above, neither 0 nor 1 works for any user who is not > using the acpi_video backlight controls for whatever reason (e.g. because > it's buggy, or simply not present at all). I'm open for suggestions on how to > fix this problem without config option (even if I think the option makes > sense), but just raising the hardcoded value from 0 to 1 definitely does not > help. > > Sebastian Kügler wrote: > What would probably make sense it to use 10% or 1 (whichever is more) as > lowest value. We do know the max value, so we can make sure it's low, but not > zero. This should be tried. > > How does raising it from 0 to 1 not help, btw? > > Danny Baumann wrote: > Raising to 10% would help in my case, but how do you suggest handling > cases where either > - 10% is considered 'too bright' (given that it was darker before - > people might complain about 'more power usage than necessary') and > - 10% is brighter than the brightness before dimming? > > The answer to 'why does raising to 1 not help' is a few comments above ;) > "That doesn't help though, as with intel-backlight 1 is still way too low > to be visible. I think the value corresponding to acpi value 0 is about 5%" > > But can you please explain where exactly you see the problem of this > option compared to e.g. the way more sophisticated 'run script' options? > > Sebastian Kügler wrote: > The option does not solve the problem. We have to make sure the system is > usable without changing an option. qMax(5%, 1) is also f
Re: Review Request 109792: Update 'dim display' algorithm.
On Thursday, April 11, 2013 17:59:03 Danny Baumann wrote: > >> Having said that, my current plan is the following: > >> - Drop the UI option > >> - However, leave the configurability via kconfig intact. That way, users > >> who are not happy with the defaults do at least have a remote chance of > >> changing it without recompiling KDE (after they found it's configurable, > >> of course - but there's a huge step between looking something up and > >> compiling a whole DE to change it) - Additionally add a dimRelative > >> kconfig setting. With dimRelative=true, dimRatio is interpreted as a > >> factor > >> (dimmedBrightness = origBrightness * dimRatio), with it being false it's > >> interpreted as an absolute value (dimmedBrightness = 100f * dimRatio). - > >> The defaults are set up as dimRatio = 0.05, dimRelative = false > >> - Do the dimming in 2 steps, the first one being done after the > >> configured > >> time, the second one 10 seconds later > >> > >> Is that approach ok? > > > > Nope, even a config key just adds maintainability and support burden. I've > > been there, done that, and didn't like the ride. > > > > The bug you are addressing is entirely fixable without an option, there is > > simply not a good reason to make this configurable. > > So you say whoever is not happy with the proposed defaults (e.g. me) is > suppposed to either recompile KDE or use another DE? You can already change the defaults with the slider offered to set the brightness. What you are proposing is an option for an option, that's one layer of complexity too much, unnecessarily. You can even define this setting differently for each activity. > My patch is _not_ > addressing a bug only (although that ultimately was the trigger), but > aims at improving the algorithm in the process. Adding complexity for no good reason (but with good reasons against it) is not improving the code, it's making it harder to understand, explain and maintain, harder to support and easier to break and perform worse (though probably not noticeable). > (In case you're wondering, I'd like to make it dim to 30% of the > original brightness to make it work seamlessly in the evening where the > non-dimmed brightness is lower. I understand this may not be deemed a > suitable default behaviour.) I haven't thought about this use case. The idea of a dimming relative to initial brightness is not bad. It would solve the case where you've set the brightness lower (to, say 50%), the display dims in steps and as first step cranks the brightness up, not down. I'm not sure this is still a bug, haven't noticed it in a while, though. A problem I can imagine with getting a relative setting to do what you expect it to is that eyes and perception of brightness (relative to ambient light, relative to what the user is used to) is not linear, but far from it. You could certainly try making the dimming relative, that's entirely safe unless a lower limit is hit. I haven't tried this, though, and as I said it's probably very complex to get right -- maybe not. When I encounter the problem you're trying to solve, it's usually in a dark environment where I switch the laptop on, and the display is too bright. The dim-on-idle doesn't matter at all in my case since I'm then actually using the machine and it won't go idle. When I'm watching a movie, I usually switch to my media consumption activity, which has it all set up (i.e. no dimming at all, switch off after a long time, etc.) and the media in front of me. It's good to try this, but I think this review request is growing quite out of proportion, and it already addresses different things (making it harder to pass review). How about we try to get the minimal solution in, and then back to the drawing board to come up with a solution that satisfies your usecase and is acceptable for everybody? That might sound more tedious, but in the end the process is quicker and more satisfying. Cheers, -- sebas http://www.kde.org | http://vizZzion.org | GPG Key ID: 9119 0EF9
Review Request 109967: Export KLocale flags to QML locale bindings correctly
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109967/ --- Review request for KDE Runtime. Description --- KLocale::TimeFormatOptions, KLocale::TimeProcessingOptions , KLocale::DateTimeComponents, KLocale::DateTimeFormatOptions are now exported to QML correctly. Just added Q_FLAGS after Q_DECLARE_FLAGS Refer to this email : http://mail.kde.org/pipermail/plasma-devel/2013-April/024753.html Diffs - plasma/declarativeimports/locale/locale_p.h 5b4668f Diff: http://git.reviewboard.kde.org/r/109967/diff/ Testing --- Tested a plasmoid that used these flags. Thanks, Anant Kamath
Re: Review Request 109967: Export KLocale flags to QML locale bindings correctly
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109967/ --- (Updated April 11, 2013, 8:25 p.m.) Review request for KDE Runtime. Description --- KLocale::TimeFormatOptions, KLocale::TimeProcessingOptions , KLocale::DateTimeComponents, KLocale::DateTimeFormatOptions are now exported to QML correctly. Just added Q_FLAGS after Q_DECLARE_FLAGS Refer to this email : http://mail.kde.org/pipermail/plasma-devel/2013-April/024753.html Diffs - plasma/declarativeimports/locale/locale_p.h 5b4668f Diff: http://git.reviewboard.kde.org/r/109967/diff/ Testing --- Tested a plasmoid that used these flags. Thanks, Anant Kamath
Re: Review Request 109967: Export KLocale flags to QML locale bindings correctly
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109967/ --- (Updated April 11, 2013, 8:30 p.m.) Review request for KDE Runtime and Plasma. Description --- KLocale::TimeFormatOptions, KLocale::TimeProcessingOptions , KLocale::DateTimeComponents, KLocale::DateTimeFormatOptions are now exported to QML correctly. Just added Q_FLAGS after Q_DECLARE_FLAGS Refer to this email : http://mail.kde.org/pipermail/plasma-devel/2013-April/024753.html Diffs - plasma/declarativeimports/locale/locale_p.h 5b4668f Diff: http://git.reviewboard.kde.org/r/109967/diff/ Testing --- Tested a plasmoid that used these flags. Thanks, Anant Kamath
Re: Review Request 109967: Export KLocale flags to QML locale bindings correctly
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109967/#review30943 --- Ship it! Ship It! - Sebastian Kügler On April 11, 2013, 8:30 p.m., Anant Kamath wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/109967/ > --- > > (Updated April 11, 2013, 8:30 p.m.) > > > Review request for KDE Runtime and Plasma. > > > Description > --- > > KLocale::TimeFormatOptions, KLocale::TimeProcessingOptions , > KLocale::DateTimeComponents, KLocale::DateTimeFormatOptions are now exported > to QML correctly. > Just added Q_FLAGS after Q_DECLARE_FLAGS > > Refer to this email : > http://mail.kde.org/pipermail/plasma-devel/2013-April/024753.html > > > Diffs > - > > plasma/declarativeimports/locale/locale_p.h 5b4668f > > Diff: http://git.reviewboard.kde.org/r/109967/diff/ > > > Testing > --- > > Tested a plasmoid that used these flags. > > > Thanks, > > Anant Kamath > >
Re: Review Request 109967: Export KLocale flags to QML locale bindings correctly
> On April 11, 2013, 8:39 p.m., Sebastian Kügler wrote: > > Ship It! This patch needs forward porting to (the Frameworks5) plasma-framework repo. Can you do this, or would you rather have me do it? - Sebastian --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109967/#review30943 --- On April 11, 2013, 8:30 p.m., Anant Kamath wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/109967/ > --- > > (Updated April 11, 2013, 8:30 p.m.) > > > Review request for KDE Runtime and Plasma. > > > Description > --- > > KLocale::TimeFormatOptions, KLocale::TimeProcessingOptions , > KLocale::DateTimeComponents, KLocale::DateTimeFormatOptions are now exported > to QML correctly. > Just added Q_FLAGS after Q_DECLARE_FLAGS > > Refer to this email : > http://mail.kde.org/pipermail/plasma-devel/2013-April/024753.html > > > Diffs > - > > plasma/declarativeimports/locale/locale_p.h 5b4668f > > Diff: http://git.reviewboard.kde.org/r/109967/diff/ > > > Testing > --- > > Tested a plasmoid that used these flags. > > > Thanks, > > Anant Kamath > >
Re: Review Request 109967: Export KLocale flags to QML locale bindings correctly
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109967/ --- (Updated April 11, 2013, 10:48 p.m.) Status -- This change has been marked as submitted. Review request for KDE Runtime and Plasma. Description --- KLocale::TimeFormatOptions, KLocale::TimeProcessingOptions , KLocale::DateTimeComponents, KLocale::DateTimeFormatOptions are now exported to QML correctly. Just added Q_FLAGS after Q_DECLARE_FLAGS Refer to this email : http://mail.kde.org/pipermail/plasma-devel/2013-April/024753.html Diffs - plasma/declarativeimports/locale/locale_p.h 5b4668f Diff: http://git.reviewboard.kde.org/r/109967/diff/ Testing --- Tested a plasmoid that used these flags. Thanks, Anant Kamath
