Re: Review Request: qml based kwin shadow
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108243/#review25138 --- This review has been submitted with commit c58962b112627de29f06e4bf21a1dce0fccab0cc by Weng Xuetian to branch KDE/4.10. - Commit Hook On Jan. 8, 2013, 12:21 a.m., Xuetian Weng wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108243/ --- (Updated Jan. 8, 2013, 12:21 a.m.) Review request for kde-workspace, kwin, Plasma, Thomas Lübking, Aaron J. Seigo, Marco Martin, and Martin Gräßlin. Description --- This is a different solution solve problem in https://git.reviewboard.kde.org/r/108224/ 1. use qml to draw shadow in DeclarativeView. 2. set blur mask svg property in qml 3. and fix some layout problem in big icons and small icons. Diffs - kwin/tabbox/declarative.cpp 3bdcfac kwin/tabbox/qml/CMakeLists.txt d4bc863 kwin/tabbox/qml/IconTabBox.qml 23bb11b kwin/tabbox/qml/ShadowedSvgItem.qml PRE-CREATION kwin/tabbox/qml/clients/big_icons/contents/ui/main.qml 7115b7f kwin/tabbox/qml/clients/compact/contents/ui/main.qml 1f6f036 kwin/tabbox/qml/clients/informative/contents/ui/main.qml 3a2c4a3 kwin/tabbox/qml/clients/present_windows/contents/ui/main.qml 14a54d3 kwin/tabbox/qml/clients/small_icons/contents/ui/main.qml ea09ed0 kwin/tabbox/qml/clients/text/contents/ui/main.qml c0def27 kwin/tabbox/qml/clients/thumbnails/contents/ui/main.qml efe3ebe kwin/tabbox/qml/tabbox.qml 4176231 Diff: http://git.reviewboard.kde.org/r/108243/diff/ Testing --- all desktop tabbox is tested with Air and Slim Glow, composite and non-composite, no problem. Thanks, Xuetian Weng
Re: KDEREVIEW: Mangonel
On Wed, Jan 09, 2013 at 09:07:13PM +0200, Yuri Chornoivan wrote: If there is no Help button and no desktop file, there is not much sense in making docbooks. I agree. I propose just add an item to UserBase launchers list [1] and some tiny page based on README.md. I'll be sure to market it wherever I can when I finally release it. :-) -- Martin Sandsmark
Review Request: fix isMinimized in KWindowInfo
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108314/ --- Review request for kdelibs, Thomas Lübking and Martin Gräßlin. Description --- Check NETWM 1.2 condition first. One case when old code doesn't work: kwin enable always keep thumbnails, kwin still use NET:Hidden in the minimized window, but mappingState is not Iconic. This would fix if kwin enable always keep thumbnails for all window, pager still show window is not minimized. Diffs - kdeui/windowmanagement/kwindowinfo_x11.cpp d983c9a Diff: http://git.reviewboard.kde.org/r/108314/diff/ Testing --- compiles and pager is fixed. Thanks, Xuetian Weng
Re: Review Request: Make Strigi optional on OS X
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108315/ --- (Updated Jan. 9, 2013, 11:30 p.m.) Review request for kdelibs. Description --- Some kde apps can work without strigi on OS X, this patch is for reduce dependencies when packaging those apps, but i don't know whether it can be optional for official kdelibs on OS X. So I put it here, please give some comments. Diffs - CMakeLists.txt f078b89 kio/kio/kfilemetainfo.h 6920ffe Diff: http://git.reviewboard.kde.org/r/108315/diff/ Testing --- Compiles. Some apps works. Thanks, Yue Liu
Re: Review Request: use _NET_WM_STATE_HIDDEN to check if the window is minimized instead of WM_STATE == ICONIC when possible.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108308/ --- (Updated Jan. 9, 2013, 11:56 p.m.) Review request for kdelibs, kwin, Aaron J. Seigo, and Martin Gräßlin. Changes --- No matter whether the wm support hidden, if nothing is set, the window is not minimized. Description --- When setting Keep window thumbnails to Always (Breaks minimization), kwin will keep WM_STATE to be NORMAL when a client is minimized while including _NET_WM_STATE_HIDDEN in its _NET_WM_STATE, as confirmed by ICCCM[1] and Extended Window Manager Hints[2]. However, apart from the expected result (breaks minimization: the client will continue to refresh its content) the minimized window is not shown as minimized in icontasks and pager. These two plasma addons (and probably other addons as well) uses KWindowInfo::isMinimized to determine whether the window is minimized. However, this function threat all window that are not Iconic (WM_STATE != ICONIC) as not minimized, in contradiction to the Extended window manager hints which says, Pagers and similar applications should use _NET_WM_STATE_HIDDEN instead of WM_STATE to decide whether to display a window in miniature representations of the windows on a desktop. This patch correct this behavior and therefore correct the behavior of both pager and icontasks in this situation. [1] http://tronche.com/gui/x/icccm/sec-4.html#s-4.1.3.1 [2] http://standards.freedesktop.org/wm-spec/wm-spec-1.3.html#id2731936 Diffs (updated) - kdeui/windowmanagement/kwindowinfo_x11.cpp d983c9a Diff: http://git.reviewboard.kde.org/r/108308/diff/ Testing --- Compiled, pager and icontasks shows minimized windows correctly. Thanks, Yichao Yu
Re: Review Request: use _NET_WM_STATE_HIDDEN to check if the window is minimized instead of WM_STATE == ICONIC when possible.
On Jan. 9, 2013, 10:45 p.m., Thomas Lübking wrote: Random addendums: - state hidden is also provided by metacity, icewm, openbox and compiz - mappingState() is part of the public API, thus not cut off (if you actually want to know it) - not unmapping windows has more issues than the pager/taskbar, so it's not like minimization wasn't still broken by the setting (non-kde/clients relying on rather the actual mapping state to stall output processing etc.) Yichao Yu wrote: (well I am not talking about removing mappingState() ... k, good to know it is public :P...) Yes I am aware of those issues by not unmapping windows (from bug report here[1], and I don't think I am using a application that have those problems), but I don't think isMinimized() does it correctly right now. [1] https://bugs.kde.org/show_bug.cgi?id=189435 Thomas Lübking wrote: This was not meant as criticism to the patch but only things others might immediately worry about as well - so they don't have to check them. What needed to be tested is for what occasions other WMs set the iconic state (which becomes a shortcut true with the patch) Could be a problem with virtual desktop or shading implementations. In this case a less invasive alternative would be to alter the taskbar/pager implementation to check KWindowInfo::state() rather then ::isMinimized() I'll test the ones mentioned above and later on e17 ... ahhh, while I'm discussing with Yichao Yu I didn't realize he also submit a patch.. but logically this patch has problem. If wm doesn't support netwm, and current mappingState is iconic. This code will get things wrong for non-minimized window. first if: false, since window is not set to Iconic second if: false, since wm is netwm 1.2 compatible return: finally return true, since it's not netwm 1.2 compatible, while it should be false. - Xuetian --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108308/#review25103 --- On Jan. 9, 2013, 9:06 p.m., Yichao Yu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108308/ --- (Updated Jan. 9, 2013, 9:06 p.m.) Review request for kdelibs, kwin and Aaron J. Seigo. Description --- When setting Keep window thumbnails to Always (Breaks minimization), kwin will keep WM_STATE to be NORMAL when a client is minimized while including _NET_WM_STATE_HIDDEN in its _NET_WM_STATE, as confirmed by ICCCM[1] and Extended Window Manager Hints[2]. However, apart from the expected result (breaks minimization: the client will continue to refresh its content) the minimized window is not shown as minimized in icontasks and pager. These two plasma addons (and probably other addons as well) uses KWindowInfo::isMinimized to determine whether the window is minimized. However, this function threat all window that are not Iconic (WM_STATE != ICONIC) as not minimized, in contradiction to the Extended window manager hints which says, Pagers and similar applications should use _NET_WM_STATE_HIDDEN instead of WM_STATE to decide whether to display a window in miniature representations of the windows on a desktop. This patch correct this behavior and therefore correct the behavior of both pager and icontasks in this situation. [1] http://tronche.com/gui/x/icccm/sec-4.html#s-4.1.3.1 [2] http://standards.freedesktop.org/wm-spec/wm-spec-1.3.html#id2731936 Diffs - kdeui/windowmanagement/kwindowinfo_x11.cpp d983c9a Diff: http://git.reviewboard.kde.org/r/108308/diff/ Testing --- Compiled, pager and icontasks shows minimized windows correctly. Thanks, Yichao Yu
Review Request: Make Strigi optional on OS X
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108315/ --- Review request for kdelibs. Description --- Some kde apps can work without strigi on OS X, this patch is for reduce dependencies when packaging those apps, but i don't know whether it can be optional for official kdelibs on OS X. So I put it here, please give some comments. Diffs - CMakeLists.txt f078b89 kio/kio/kfilemetainfo.h 6920ffe Diff: http://git.reviewboard.kde.org/r/108315/diff/ Testing --- Compiles. Some apps works. Thanks, Yue Liu
Re: Review Request: use _NET_WM_STATE_HIDDEN to check if the window is minimized instead of WM_STATE == ICONIC when possible.
On Jan. 9, 2013, 10:45 p.m., Thomas Lübking wrote: Random addendums: - state hidden is also provided by metacity, icewm, openbox and compiz - mappingState() is part of the public API, thus not cut off (if you actually want to know it) - not unmapping windows has more issues than the pager/taskbar, so it's not like minimization wasn't still broken by the setting (non-kde/clients relying on rather the actual mapping state to stall output processing etc.) Yichao Yu wrote: (well I am not talking about removing mappingState() ... k, good to know it is public :P...) Yes I am aware of those issues by not unmapping windows (from bug report here[1], and I don't think I am using a application that have those problems), but I don't think isMinimized() does it correctly right now. [1] https://bugs.kde.org/show_bug.cgi?id=189435 Thomas Lübking wrote: This was not meant as criticism to the patch but only things others might immediately worry about as well - so they don't have to check them. What needed to be tested is for what occasions other WMs set the iconic state (which becomes a shortcut true with the patch) Could be a problem with virtual desktop or shading implementations. In this case a less invasive alternative would be to alter the taskbar/pager implementation to check KWindowInfo::state() rather then ::isMinimized() I'll test the ones mentioned above and later on e17 Xuetian Weng wrote: ... ahhh, while I'm discussing with Yichao Yu I didn't realize he also submit a patch.. but logically this patch has problem. If wm doesn't support netwm, and current mappingState is iconic. This code will get things wrong for non-minimized window. first if: false, since window is not set to Iconic second if: false, since wm is netwm 1.2 compatible return: finally return true, since it's not netwm 1.2 compatible, while it should be false. ah sorry, a typo: and current mappingState is NOT iconic. - Xuetian --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108308/#review25103 --- On Jan. 9, 2013, 9:06 p.m., Yichao Yu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108308/ --- (Updated Jan. 9, 2013, 9:06 p.m.) Review request for kdelibs, kwin and Aaron J. Seigo. Description --- When setting Keep window thumbnails to Always (Breaks minimization), kwin will keep WM_STATE to be NORMAL when a client is minimized while including _NET_WM_STATE_HIDDEN in its _NET_WM_STATE, as confirmed by ICCCM[1] and Extended Window Manager Hints[2]. However, apart from the expected result (breaks minimization: the client will continue to refresh its content) the minimized window is not shown as minimized in icontasks and pager. These two plasma addons (and probably other addons as well) uses KWindowInfo::isMinimized to determine whether the window is minimized. However, this function threat all window that are not Iconic (WM_STATE != ICONIC) as not minimized, in contradiction to the Extended window manager hints which says, Pagers and similar applications should use _NET_WM_STATE_HIDDEN instead of WM_STATE to decide whether to display a window in miniature representations of the windows on a desktop. This patch correct this behavior and therefore correct the behavior of both pager and icontasks in this situation. [1] http://tronche.com/gui/x/icccm/sec-4.html#s-4.1.3.1 [2] http://standards.freedesktop.org/wm-spec/wm-spec-1.3.html#id2731936 Diffs - kdeui/windowmanagement/kwindowinfo_x11.cpp d983c9a Diff: http://git.reviewboard.kde.org/r/108308/diff/ Testing --- Compiled, pager and icontasks shows minimized windows correctly. Thanks, Yichao Yu
Re: Review Request: use _NET_WM_STATE_HIDDEN to check if the window is minimized instead of WM_STATE == ICONIC when possible.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108308/ --- (Updated Jan. 10, 2013, 12:09 a.m.) Review request for kdelibs, kwin, Aaron J. Seigo, and Martin Gräßlin. Changes --- so it is from https://git.reviewboard.kde.org/r/108314/ Description --- When setting Keep window thumbnails to Always (Breaks minimization), kwin will keep WM_STATE to be NORMAL when a client is minimized while including _NET_WM_STATE_HIDDEN in its _NET_WM_STATE, as confirmed by ICCCM[1] and Extended Window Manager Hints[2]. However, apart from the expected result (breaks minimization: the client will continue to refresh its content) the minimized window is not shown as minimized in icontasks and pager. These two plasma addons (and probably other addons as well) uses KWindowInfo::isMinimized to determine whether the window is minimized. However, this function threat all window that are not Iconic (WM_STATE != ICONIC) as not minimized, in contradiction to the Extended window manager hints which says, Pagers and similar applications should use _NET_WM_STATE_HIDDEN instead of WM_STATE to decide whether to display a window in miniature representations of the windows on a desktop. This patch correct this behavior and therefore correct the behavior of both pager and icontasks in this situation. [1] http://tronche.com/gui/x/icccm/sec-4.html#s-4.1.3.1 [2] http://standards.freedesktop.org/wm-spec/wm-spec-1.3.html#id2731936 Diffs (updated) - kdeui/windowmanagement/kwindowinfo_x11.cpp d983c9a Diff: http://git.reviewboard.kde.org/r/108308/diff/ Testing --- Compiled, pager and icontasks shows minimized windows correctly. Thanks, Yichao Yu
Re: Review Request: use _NET_WM_STATE_HIDDEN to check if the window is minimized instead of WM_STATE == ICONIC when possible.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108308/ --- (Updated Jan. 10, 2013, 12:12 a.m.) Review request for kdelibs, kwin, Aaron J. Seigo, and Martin Gräßlin. Description --- When setting Keep window thumbnails to Always (Breaks minimization), kwin will keep WM_STATE to be NORMAL when a client is minimized while including _NET_WM_STATE_HIDDEN in its _NET_WM_STATE, as confirmed by ICCCM[1] and Extended Window Manager Hints[2]. However, apart from the expected result (breaks minimization: the client will continue to refresh its content) the minimized window is not shown as minimized in icontasks and pager. These two plasma addons (and probably other addons as well) uses KWindowInfo::isMinimized to determine whether the window is minimized. However, this function threat all window that are not Iconic (WM_STATE != ICONIC) as not minimized, in contradiction to the Extended window manager hints which says, Pagers and similar applications should use _NET_WM_STATE_HIDDEN instead of WM_STATE to decide whether to display a window in miniature representations of the windows on a desktop. This patch correct this behavior and therefore correct the behavior of both pager and icontasks in this situation. [1] http://tronche.com/gui/x/icccm/sec-4.html#s-4.1.3.1 [2] http://standards.freedesktop.org/wm-spec/wm-spec-1.3.html#id2731936 Diffs - kdeui/windowmanagement/kwindowinfo_x11.cpp d983c9a Diff: http://git.reviewboard.kde.org/r/108308/diff/ Testing (updated) --- Compiled, pager and icontasks shows minimized windows correctly. Also tested on openbox (+plasma's pager) by Xuetian Weng. Thanks, Yichao Yu
Re: Review Request: Make Strigi optional on OS X
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108315/ --- (Updated Jan. 10, 2013, 3:49 a.m.) Review request for kdelibs. Description --- Some kde apps can work without strigi on OS X, this patch is for reduce dependencies when packaging those apps, but i don't know whether it can be optional for official kdelibs on OS X. So I put it here, please give some comments. Diffs (updated) - CMakeLists.txt 5df26e5 kde3support/CMakeLists.txt 2e91b73 kdewidgets/CMakeLists.txt 5153601 kfile/CMakeLists.txt ceae140 khtml/CMakeLists.txt 99034cc kio/kio/kfilemetainfo.h 6920ffe kioslave/metainfo/CMakeLists.txt cbf2d86 kparts/CMakeLists.txt 2eab2e8 Diff: http://git.reviewboard.kde.org/r/108315/diff/ Testing --- Compiles. Some apps works. Thanks, Yue Liu
Re: Review Request: Make Strigi optional on OS X
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108315/#review25141 --- AFAIK, Strigi is only used if KFileMetaInfo in kdelibs. Removing it will break the following - http://lxr.kde.org/ident?i=KFileMetaInfo The most notable of these is wallpaper handling in Plasma. I'm not sure if it will break - but it won't display the resolution of the images when choosing a wallpaper. You could also possibly do the same in kde-runtime. There seems to be a trash analyzer for strigi, but that hasn't been installed since 2007 - commit f3578b6c8c27ff1808cb464e5a95fc803e6c84b0 Author: Laurent Montel mon...@kde.org Date: Fri Apr 20 10:06:23 2007 + Don't install it for the moment need to fix it svn path=/trunk/KDE/kdebase/runtime/; revision=656108 I wonder why it is still being compiled. If it is not required, then it can be thrown away and strigi can be removed as a dependency from kde-runtime completely (Linux, Mac and Windows) - Vishesh Handa On Jan. 10, 2013, 3:49 a.m., Yue Liu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108315/ --- (Updated Jan. 10, 2013, 3:49 a.m.) Review request for kdelibs. Description --- Some kde apps can work without strigi on OS X, this patch is for reduce dependencies when packaging those apps, but i don't know whether it can be optional for official kdelibs on OS X. So I put it here, please give some comments. Diffs - CMakeLists.txt 5df26e5 kde3support/CMakeLists.txt 2e91b73 kdewidgets/CMakeLists.txt 5153601 kfile/CMakeLists.txt ceae140 khtml/CMakeLists.txt 99034cc kio/kio/kfilemetainfo.h 6920ffe kioslave/metainfo/CMakeLists.txt cbf2d86 kparts/CMakeLists.txt 2eab2e8 Diff: http://git.reviewboard.kde.org/r/108315/diff/ Testing --- Compiles. Some apps works. Thanks, Yue Liu
Re: Review Request: use _NET_WM_STATE_HIDDEN to check if the window is minimized instead of WM_STATE == ICONIC when possible.
On Jan. 10, 2013, 6:42 a.m., Martin Gräßlin wrote: the big question is whether the code has been like that for historic reasons or whether there are problems with some window managers. I am for 99 % sure that it's for historic reasons, but the remaining 1 % tells me that it's dangerous to go into 4.10 (we hardly test with other WMs any more). Also given the fact that it has been broken for quite some time (and only with a non-default option in KWin), tells me that it's not like immediate action needed. Personally I would say the best we can do is introducing a proper _NET_WM_STATE_MINIMIZED which we set for minimized windows. Not sure whether we could agree on that in the standard, but at least we could add a KDE hint. We see the problem with _NET_WM_STATE_HIDDEN in the comment section of the code: shaded windows use it, too. plus as mentioned before, taskbar pager can still use an altered version of the function (keeps any regression local) i'm at 99.9% though ;-) - Thomas --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108308/#review25123 --- On Jan. 10, 2013, 6:50 a.m., Yichao Yu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108308/ --- (Updated Jan. 10, 2013, 6:50 a.m.) Review request for kdelibs, kwin, Plasma, Aaron J. Seigo, and Martin Gräßlin. Description --- When setting Keep window thumbnails to Always (Breaks minimization), kwin will keep WM_STATE to be NORMAL when a client is minimized while including _NET_WM_STATE_HIDDEN in its _NET_WM_STATE, as confirmed by ICCCM[1] and Extended Window Manager Hints[2]. However, apart from the expected result (breaks minimization: the client will continue to refresh its content) the minimized window is not shown as minimized in icontasks and pager. These two plasma addons (and probably other addons as well) uses KWindowInfo::isMinimized to determine whether the window is minimized. However, this function threat all window that are not Iconic (WM_STATE != ICONIC) as not minimized, in contradiction to the Extended window manager hints which says, Pagers and similar applications should use _NET_WM_STATE_HIDDEN instead of WM_STATE to decide whether to display a window in miniature representations of the windows on a desktop. This patch correct this behavior and therefore correct the behavior of both pager and icontasks in this situation. [1] http://tronche.com/gui/x/icccm/sec-4.html#s-4.1.3.1 [2] http://standards.freedesktop.org/wm-spec/wm-spec-1.3.html#id2731936 Diffs - kdeui/windowmanagement/kwindowinfo_x11.cpp d983c9a Diff: http://git.reviewboard.kde.org/r/108308/diff/ Testing --- Compiled, pager and icontasks shows minimized windows correctly. Also tested on openbox (+plasma's pager) by Xuetian Weng. Thanks, Yichao Yu
Re: Review Request: fix 234407 - set minimum width for system settings icon view
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108328/#review25177 --- systemsettings/icons/FileItemDelegate.cpp http://git.reviewboard.kde.org/r/108328/#comment19247 relict?! systemsettings/icons/FileItemDelegate.cpp http://git.reviewboard.kde.org/r/108328/#comment19245 , m_minimumSize(0,0) systemsettings/icons/FileItemDelegate.cpp http://git.reviewboard.kde.org/r/108328/#comment19246 return KFileItemDelegate::sizeHint(option, index).expandedTo(m_minimumSize); systemsettings/icons/IconMode.cpp http://git.reviewboard.kde.org/r/108328/#comment19248 should this not rely on the icon size rather than on the fontmetrics (only)? The point is to get more size between the icons, i thought ;-) - Thomas Lübking On Jan. 10, 2013, 6:09 p.m., Xuetian Weng wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108328/ --- (Updated Jan. 10, 2013, 6:09 p.m.) Review request for kde-workspace, Ben Cooksley, Thomas Lübking, Chao Feng, and Yichao Yu. Description --- Set minimum width for system settings icon view item depending on font. Since KFileItemDelegate doesn't provides setMinimumSize, we make a sub-class that can have a minimumSize. (Maybe should be add to kdelibs in the future?) This fontHeight * 6 heuristic value works for all languages. CJK character is usually square (width = height) so this roughly means 6 CJK character, and 12 latin letter (height = width * 2), which will always look good. To fengchao, I'm sorry if you feel I steal your job.. but I can't stop myself since I think I can fix it in 10 minutes.. This addresses bug 234407. http://bugs.kde.org/show_bug.cgi?id=234407 Diffs - systemsettings/icons/CMakeLists.txt 0830dd7 systemsettings/icons/FileItemDelegate.h PRE-CREATION systemsettings/icons/FileItemDelegate.cpp PRE-CREATION systemsettings/icons/IconMode.cpp 37cfc4b Diff: http://git.reviewboard.kde.org/r/108328/diff/ Testing --- see screenshot. Screenshots --- english after change http://git.reviewboard.kde.org/r/108328/s/1014/ chinese after change http://git.reviewboard.kde.org/r/108328/s/1015/ chinese before change http://git.reviewboard.kde.org/r/108328/s/1016/ spanish after change http://git.reviewboard.kde.org/r/108328/s/1017/ Thanks, Xuetian Weng
Re: Review Request: fix 234407 - set minimum width for system settings icon view
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108328/#review25179 --- Screenshot: spanish after change http://git.reviewboard.kde.org//r/108328/#scomment129 Is gest... elided before the patch as well? (got no es locales installed) - Thomas Lübking On Jan. 10, 2013, 6:09 p.m., Xuetian Weng wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108328/ --- (Updated Jan. 10, 2013, 6:09 p.m.) Review request for kde-workspace, Ben Cooksley, Thomas Lübking, Chao Feng, and Yichao Yu. Description --- Set minimum width for system settings icon view item depending on font. Since KFileItemDelegate doesn't provides setMinimumSize, we make a sub-class that can have a minimumSize. (Maybe should be add to kdelibs in the future?) This fontHeight * 6 heuristic value works for all languages. CJK character is usually square (width = height) so this roughly means 6 CJK character, and 12 latin letter (height = width * 2), which will always look good. To fengchao, I'm sorry if you feel I steal your job.. but I can't stop myself since I think I can fix it in 10 minutes.. This addresses bug 234407. http://bugs.kde.org/show_bug.cgi?id=234407 Diffs - systemsettings/icons/CMakeLists.txt 0830dd7 systemsettings/icons/FileItemDelegate.h PRE-CREATION systemsettings/icons/FileItemDelegate.cpp PRE-CREATION systemsettings/icons/IconMode.cpp 37cfc4b Diff: http://git.reviewboard.kde.org/r/108328/diff/ Testing --- see screenshot. Screenshots --- english after change http://git.reviewboard.kde.org/r/108328/s/1014/ chinese after change http://git.reviewboard.kde.org/r/108328/s/1015/ chinese before change http://git.reviewboard.kde.org/r/108328/s/1016/ spanish after change http://git.reviewboard.kde.org/r/108328/s/1017/ Thanks, Xuetian Weng
New lockscreen
Hi! The new lock screen has some more or less serious regressions, and doesn't seem to be maintained by anyone in particular (one of the regression bugs filed against it is from november, and I don't really see anyone in particular commenting or fixing anything, it only got a handful of commits in december). So, who is supposed maintain this new screenlocker? In its current state it provides nothing new over the old lock screen (for users), but has about a dozen (12 filed ones, maybe more unreported ones?) regressions. A list of the current regressions are available here: https://bugs.kde.org/buglist.cgi?product=kscreensavercomponent=locker-qmlbug_status=UNCONFIRMEDbug_status=CONFIRMEDbug_status=ASSIGNEDbug_status=REOPENEDlist_id=384099 I can help fix some of them, but I know nothing about the codebase, and not really much about QML, so I would at least appreciate some help. Another alternative is to revert the replacement for KDE 4.10, and instead postpone it for KDE 4.11, though that's not nice considering how it has been marketed already. But IMHO it's better than shipping it in its current state so that people won't think that QML == regressions and brokenness. -- Martin Sandsmark KDE
Re: Review Request: Fix bug 234407 - systemsettings iconview wordwrapping cause narrow icon in zhcn locale
On Jan. 9, 2013, 10:09 a.m., Ben Cooksley wrote: systemsettings/icons/IconMode.cpp, line 183 http://git.reviewboard.kde.org/r/108285/diff/1/?file=106110#file106110line183 Not sure I like the idea of a hardcoded list of languages... is there a better way of determining if a language is CJK? Christoph Feck wrote: CJK is actually naming the languages which use CJK, so the list is hardcoded by definition. Yichao Yu wrote: I guess the question is not which languages are CJK but which languages have this problem. Yichao Yu wrote: Plus, there may be english text even when the current locale is cjk[1], so I really don't think deciding from the current locale is a good idea. Personally, I like what I am having now (larger spacing?). I don't know what it looks like without word wrap but I think multiple lines is better if the text is really too long (e.g. ?). I did have a suggestion on how the word wrap should be done for cjk (see the last few lines in the description of this bug[2]) i.e. it may be better to keep each lines roughly the same length. For detecting whether word wrapping should be used (and probably what method should be used in order to have a better appearance e.g. using the alternative method for CJK I mentioned above), I think it is probably a better way to detect blank space in the text. It might be a better idea to increase the threshold (maximum length) if there is not a single space in the text and use some better method to do word wrapping in such case. This may work for any language that allow word wrapping (I personally don't know any language that does not, correct me if I am wrong.) (including English (for extremely long words) if you add - correctly). [1] http://wstaw.org/m/2013/01/09/plasma-desktopr20016.png [2] https://bugs.kde.org/show_bug.cgi?id=309780 Xuetian Weng wrote: 1. You are missing zh_HK. 2. Have you ever consider if a non-translated long-english string appear in systemsettings (3rd party, maybe), how would your patch affect the appearance? What I would suggest is set a minimum width for delegate (for all locale), you can use fontMetrics() * constant number, which you can sure the width is longer than maybe 6-8 CJK character, and you will still have good looking for non-CJK gui. I disable wordwrap mode for CJK language because this mode is just a regression for these language. I still clearly remember the day when I upgrade my KDE and open system setting, it looks so ugly :) If it is a regression, than revert it is just a simple way. The old way at least works fine for a long time. It seems most people do not like this simple revert. Here are two possible solution: 1. Set different delegate width: a. If all is non-CJK, no change needed. b. (Not true for most cases) If all items are translated into local CJK language, set width larger c. (Most cases) There are both translated and non-translated items, it is hard to make a decision. For me, I still prefer single line display. But others prefer setting a minimum width, which may make some long English word look bad. Then another solution: 2. Add a configuration option. Let the user choose what they want. - Chao --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108285/#review25038 --- On Jan. 9, 2013, 4:33 a.m., Chao Feng wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108285/ --- (Updated Jan. 9, 2013, 4:33 a.m.) Review request for kde-workspace. Description --- CJK languages do not use space as words seperator. And a CJK translation of the text in Systemsettings are very short. A single line is enough for them. This addresses bug 234407. http://bugs.kde.org/show_bug.cgi?id=234407 Diffs - systemsettings/icons/IconMode.cpp 37cfc4bed42e4d05fc4c01008f8ca2c63b287b5e Diff: http://git.reviewboard.kde.org/r/108285/diff/ Testing --- 1. Apply patch 2. Systemsetting show ok on CJK Thanks, Chao Feng
Re: Review Request: Fix bug 234407 - systemsettings iconview wordwrapping cause narrow icon in zhcn locale
On Jan. 9, 2013, 6:43 p.m., Albert Astals Cid wrote: To be honest i don't think this is the correct fix, the correct fix is specifying a minimum space between the items or a minimum size of the items themselves, not forcing the wordwrapping off Thomas Lübking wrote: itemsize (spacing would also increase space between text) and it's actually the only way to fix the bug (look at the cn text of notifications or a11y) and scales better when eg. egypt returned to hieroglyphs or so ;-) I will try out this method if I have time. If a minimum size is set, the problem of CJK language is indeed solved. But it will change the display of other languages. For this kind of change, all major languages need to checked to prevent another regression. - Chao --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108285/#review25091 --- On Jan. 9, 2013, 4:33 a.m., Chao Feng wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108285/ --- (Updated Jan. 9, 2013, 4:33 a.m.) Review request for kde-workspace. Description --- CJK languages do not use space as words seperator. And a CJK translation of the text in Systemsettings are very short. A single line is enough for them. This addresses bug 234407. http://bugs.kde.org/show_bug.cgi?id=234407 Diffs - systemsettings/icons/IconMode.cpp 37cfc4bed42e4d05fc4c01008f8ca2c63b287b5e Diff: http://git.reviewboard.kde.org/r/108285/diff/ Testing --- 1. Apply patch 2. Systemsetting show ok on CJK Thanks, Chao Feng
Review Request: fix 234407 - set minimum width for system settings icon view
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108328/ --- Review request for kde-workspace, Ben Cooksley, Thomas Lübking, Chao Feng, and Yichao Yu. Description --- have a minimum width for system settings icon view depends on font. This fontHeight * 6 heuristic value works all languages. CJK character is usually square so this roughly means 6 CJK character, and 12 latin letter, which will always look good. And to fengchao, sorry if you feel I steal your job.. but I can't stop but self since I think I can finish it in 10 minutes.. This addresses bug 234407. http://bugs.kde.org/show_bug.cgi?id=234407 Diffs - systemsettings/icons/CMakeLists.txt 0830dd7 systemsettings/icons/FileItemDelegate.h PRE-CREATION systemsettings/icons/FileItemDelegate.cpp PRE-CREATION systemsettings/icons/IconMode.cpp 37cfc4b Diff: http://git.reviewboard.kde.org/r/108328/diff/ Testing --- see screenshot. Screenshots --- english after change http://git.reviewboard.kde.org/r/108328/s/1014/ chinese after change http://git.reviewboard.kde.org/r/108328/s/1015/ chinese before change http://git.reviewboard.kde.org/r/108328/s/1016/ spanish after change http://git.reviewboard.kde.org/r/108328/s/1017/ Thanks, Xuetian Weng
Re: Review Request: fix 234407 - set minimum width for system settings icon view
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108328/ --- (Updated Jan. 10, 2013, 6:09 p.m.) Review request for kde-workspace, Ben Cooksley, Thomas Lübking, Chao Feng, and Yichao Yu. Description (updated) --- Set minimum width for system settings icon view item depending on font. Since KFileItemDelegate doesn't provides setMinimumSize, we make a sub-class that can have a minimumSize. (Maybe should be add to kdelibs in the future?) This fontHeight * 6 heuristic value works for all languages. CJK character is usually square (width = height) so this roughly means 6 CJK character, and 12 latin letter (height = width * 2), which will always look good. To fengchao, I'm sorry if you feel I steal your job.. but I can't stop myself since I think I can fix it in 10 minutes.. This addresses bug 234407. http://bugs.kde.org/show_bug.cgi?id=234407 Diffs - systemsettings/icons/CMakeLists.txt 0830dd7 systemsettings/icons/FileItemDelegate.h PRE-CREATION systemsettings/icons/FileItemDelegate.cpp PRE-CREATION systemsettings/icons/IconMode.cpp 37cfc4b Diff: http://git.reviewboard.kde.org/r/108328/diff/ Testing --- see screenshot. Screenshots --- english after change http://git.reviewboard.kde.org/r/108328/s/1014/ chinese after change http://git.reviewboard.kde.org/r/108328/s/1015/ chinese before change http://git.reviewboard.kde.org/r/108328/s/1016/ spanish after change http://git.reviewboard.kde.org/r/108328/s/1017/ Thanks, Xuetian Weng
Re: Review Request: Fix bug 234407 - systemsettings iconview wordwrapping cause narrow icon in zhcn locale
On Jan. 9, 2013, 10:09 a.m., Ben Cooksley wrote: systemsettings/icons/IconMode.cpp, line 183 http://git.reviewboard.kde.org/r/108285/diff/1/?file=106110#file106110line183 Not sure I like the idea of a hardcoded list of languages... is there a better way of determining if a language is CJK? Christoph Feck wrote: CJK is actually naming the languages which use CJK, so the list is hardcoded by definition. Yichao Yu wrote: I guess the question is not which languages are CJK but which languages have this problem. Yichao Yu wrote: Plus, there may be english text even when the current locale is cjk[1], so I really don't think deciding from the current locale is a good idea. Personally, I like what I am having now (larger spacing?). I don't know what it looks like without word wrap but I think multiple lines is better if the text is really too long (e.g. ?). I did have a suggestion on how the word wrap should be done for cjk (see the last few lines in the description of this bug[2]) i.e. it may be better to keep each lines roughly the same length. For detecting whether word wrapping should be used (and probably what method should be used in order to have a better appearance e.g. using the alternative method for CJK I mentioned above), I think it is probably a better way to detect blank space in the text. It might be a better idea to increase the threshold (maximum length) if there is not a single space in the text and use some better method to do word wrapping in such case. This may work for any language that allow word wrapping (I personally don't know any language that does not, correct me if I am wrong.) (including English (for extremely long words) if you add - correctly). [1] http://wstaw.org/m/2013/01/09/plasma-desktopr20016.png [2] https://bugs.kde.org/show_bug.cgi?id=309780 Xuetian Weng wrote: 1. You are missing zh_HK. 2. Have you ever consider if a non-translated long-english string appear in systemsettings (3rd party, maybe), how would your patch affect the appearance? What I would suggest is set a minimum width for delegate (for all locale), you can use fontMetrics() * constant number, which you can sure the width is longer than maybe 6-8 CJK character, and you will still have good looking for non-CJK gui. Chao Feng wrote: I disable wordwrap mode for CJK language because this mode is just a regression for these language. I still clearly remember the day when I upgrade my KDE and open system setting, it looks so ugly :) If it is a regression, than revert it is just a simple way. The old way at least works fine for a long time. It seems most people do not like this simple revert. Here are two possible solution: 1. Set different delegate width: a. If all is non-CJK, no change needed. b. (Not true for most cases) If all items are translated into local CJK language, set width larger c. (Most cases) There are both translated and non-translated items, it is hard to make a decision. For me, I still prefer single line display. But others prefer setting a minimum width, which may make some long English word look bad. Then another solution: 2. Add a configuration option. Let the user choose what they want. I still clearly remember the day July, 19. in 2009? which may make some long English word look bad. Why? The minimum width is hardly a limit for a long (english) string. Have a look at https://git.reviewboard.kde.org/r/108328/s/1015/ The info icon has quite some glyphs even chinese, ie. CJK does not gurantee short strings - correct? Not using a fixed size grid (but icons of variable width) can be considered a usability bug, for sure it looks dull. As consequence you can have one item with a long line determine the width of all icons and by this cause a pretty spare view. That's hardly a solution either. The problem (assumed from western esthetics) with the chinese glyphs in that screenshot is the wraps for one of them (or maybe two) what could be avoided by relaxed maximum bounds. You'd have to (in the reimplemented ::paint() function of QItemDelegate) match the width of the remaining words against the textrect width, if it's not much more (tm) you'd paint it ignoring clips, otherwise break at the flooring word, move to the next line and go on with the rest of the string. - Thomas --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108285/#review25038 --- On Jan. 9, 2013, 4:33 a.m., Chao Feng wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108285/
Re: New lockscreen
On Thursday 10 January 2013, Martin Sandsmark wrote: Hi! The new lock screen has some more or less serious regressions, and doesn't seem to be maintained by anyone in particular (one of the regression bugs filed against it is from november, and I don't really see anyone in particular commenting or fixing anything, it only got a handful of commits in december). So, who is supposed maintain this new screenlocker? In its current state it provides nothing new over the old lock screen (for users), but has about a dozen (12 filed ones, maybe more unreported ones?) regressions. A list of the current regressions are available here: https://bugs.kde.org/buglist.cgi?product=kscreensavercomponent=locker-qml; bug_status=UNCONFIRMEDbug_status=CONFIRMEDbug_status=ASSIGNEDbug_status= REOPENEDlist_id=384099 311571 and 312427 should be fixed now some bugs seems easy, some i can'r reproduce them at all. however not all of those are valid i think (the concept is: is a screenlocker, not a screensaver anymore, thus makes behavior changes) Cheers, Marco Martin
Re: New lockscreen
On Thursday, January 10, 2013 19:37:57 Martin Sandsmark wrote: So, who is supposed maintain this new screenlocker? In its current state it .. Another alternative is to revert the replacement for KDE 4.10, and instead disclaimer: i am not the maintainer of this component. i have contributed some minor things to its development. however, i'm obviously vested in the workspace in general. reverting is not going to happen at this point. but what i will do is spend tomorrow (and if necessary the next day) going through the various bug reports and fixing as many as is possible. none of them look attrociously difficult. (now, if that's all you care about, you can stop reading right here. if you care about directional issues in KDE, read on at your own risk :) after reading this email, a comment i saw on irc before checking my emails suddenly makes sense and i am deeply sadened by it. :/ i will put time in to address the list of bug reports because i care about the quality of the release. however, i do so in spite of the email sent. it is now the second time this week we've witnessed this approach at problem identification and solution, and certainly more times in the past: * CC multiple lists rather than send it to the relevant one * assume the worst and make assertions based on that (e.g. there are open bug reports == there is no maintainer) * put reversion as one of the only (if not the only) semi-realistic option offered. starting with a worst case, but probably low effort, solution is a good way to simultaneously lower morale (let's throw your effort out) and feed laziness (we can throw it out, so you don't need to work on it more). it makes us prone to take the worst case solution rather than focus on improvement. * spend more words on finding someone to accept responsibility (where's the maintainer) than defining problem solution. iow, we focus on the person rather than the product. * assumptions and conclusions about reactions that have not happened yet (QML == regressions and brokenness). * do it all at the 11th hour. one might think our development cycle consists of a few people working for months on as much as they can and then people jumping in at the last minute with Big Massive Problems They Finally Decide To Raise(tm). it's almost like we don't have a feature freeze and stabilization period that people participate in. i also have the suspicion that if this was being dealt with in person, it would have been said differently. somehow, though, because its an email and people aren't actually looking each other in the eye this is what we get. second time this week. meh. besides the brokenness of the approach, i'll let everyone in on what is probably not a big secret: for the first time since 4.0, there was essentially zero project management in the release cycle for the workspaces codebase. this came about after a group of people requested it be that way; despite attempts at reasoning it through with them they maintained that position. i'm hoping the people involved will realize just how untenable that position is now. where i failed with reason, i hope that experience will be an effective teacher. let's make this *not* the future of KDE development. -- Aaron J. Seigo signature.asc Description: This is a digitally signed message part.
Re: Review Request: fix 234407 - set minimum width for system settings icon view
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108328/ --- (Updated Jan. 10, 2013, 8:47 p.m.) Review request for kde-workspace, Ben Cooksley, Thomas Lübking, Chao Feng, and Yichao Yu. Description --- Set minimum width for system settings icon view item depending on font. Since KFileItemDelegate doesn't provides setMinimumSize, we make a sub-class that can have a minimumSize. (Maybe should be add to kdelibs in the future?) This fontHeight * 6 heuristic value works for all languages. CJK character is usually square (width = height) so this roughly means 6 CJK character, and 12 latin letter (height = width * 2), which will always look good. To fengchao, I'm sorry if you feel I steal your job.. but I can't stop myself since I think I can fix it in 10 minutes.. This addresses bug 234407. http://bugs.kde.org/show_bug.cgi?id=234407 Diffs (updated) - systemsettings/icons/CMakeLists.txt 0830dd7 systemsettings/icons/FileItemDelegate.h PRE-CREATION systemsettings/icons/FileItemDelegate.cpp PRE-CREATION systemsettings/icons/IconMode.cpp 37cfc4b Diff: http://git.reviewboard.kde.org/r/108328/diff/ Testing --- see screenshot. Screenshots --- english after change http://git.reviewboard.kde.org/r/108328/s/1014/ chinese after change http://git.reviewboard.kde.org/r/108328/s/1015/ chinese before change http://git.reviewboard.kde.org/r/108328/s/1016/ spanish after change http://git.reviewboard.kde.org/r/108328/s/1017/ Thanks, Xuetian Weng
Re: Review Request: fix 234407 - set minimum width for system settings icon view
On Jan. 10, 2013, 6:13 p.m., Thomas Lübking wrote: Screenshot: spanish after change http://git.reviewboard.kde.org Is gest... elided before the patch as well? (got no es locales installed) Yes.. I choose es as example here since I know es string is usually longer than English and I want to check how does this patch affect it. - Xuetian --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108328/#review25179 --- On Jan. 10, 2013, 8:47 p.m., Xuetian Weng wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108328/ --- (Updated Jan. 10, 2013, 8:47 p.m.) Review request for kde-workspace, Ben Cooksley, Thomas Lübking, Chao Feng, and Yichao Yu. Description --- Set minimum width for system settings icon view item depending on font. Since KFileItemDelegate doesn't provides setMinimumSize, we make a sub-class that can have a minimumSize. (Maybe should be add to kdelibs in the future?) This fontHeight * 6 heuristic value works for all languages. CJK character is usually square (width = height) so this roughly means 6 CJK character, and 12 latin letter (height = width * 2), which will always look good. To fengchao, I'm sorry if you feel I steal your job.. but I can't stop myself since I think I can fix it in 10 minutes.. This addresses bug 234407. http://bugs.kde.org/show_bug.cgi?id=234407 Diffs - systemsettings/icons/CMakeLists.txt 0830dd7 systemsettings/icons/FileItemDelegate.h PRE-CREATION systemsettings/icons/FileItemDelegate.cpp PRE-CREATION systemsettings/icons/IconMode.cpp 37cfc4b Diff: http://git.reviewboard.kde.org/r/108328/diff/ Testing --- see screenshot. Screenshots --- english after change http://git.reviewboard.kde.org/r/108328/s/1014/ chinese after change http://git.reviewboard.kde.org/r/108328/s/1015/ chinese before change http://git.reviewboard.kde.org/r/108328/s/1016/ spanish after change http://git.reviewboard.kde.org/r/108328/s/1017/ Thanks, Xuetian Weng
Re: Review Request: fix 234407 - set minimum width for system settings icon view
On Jan. 10, 2013, 6:12 p.m., Thomas Lübking wrote: systemsettings/icons/IconMode.cpp, line 182 http://git.reviewboard.kde.org/r/108328/diff/1/?file=106487#file106487line182 should this not rely on the icon size rather than on the fontmetrics (only)? The point is to get more size between the icons, i thought ;-) no, it's to have more space for the string, and to make it less wrapped, Current code doesn't work well since it seems trying to shrink the string as much as possible, and CJK character doesn't break in to non-wrappable string like English so it doesn't work well. It doesn't matter how big is your icon. - Xuetian --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108328/#review25177 --- On Jan. 10, 2013, 8:47 p.m., Xuetian Weng wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108328/ --- (Updated Jan. 10, 2013, 8:47 p.m.) Review request for kde-workspace, Ben Cooksley, Thomas Lübking, Chao Feng, and Yichao Yu. Description --- Set minimum width for system settings icon view item depending on font. Since KFileItemDelegate doesn't provides setMinimumSize, we make a sub-class that can have a minimumSize. (Maybe should be add to kdelibs in the future?) This fontHeight * 6 heuristic value works for all languages. CJK character is usually square (width = height) so this roughly means 6 CJK character, and 12 latin letter (height = width * 2), which will always look good. To fengchao, I'm sorry if you feel I steal your job.. but I can't stop myself since I think I can fix it in 10 minutes.. This addresses bug 234407. http://bugs.kde.org/show_bug.cgi?id=234407 Diffs - systemsettings/icons/CMakeLists.txt 0830dd7 systemsettings/icons/FileItemDelegate.h PRE-CREATION systemsettings/icons/FileItemDelegate.cpp PRE-CREATION systemsettings/icons/IconMode.cpp 37cfc4b Diff: http://git.reviewboard.kde.org/r/108328/diff/ Testing --- see screenshot. Screenshots --- english after change http://git.reviewboard.kde.org/r/108328/s/1014/ chinese after change http://git.reviewboard.kde.org/r/108328/s/1015/ chinese before change http://git.reviewboard.kde.org/r/108328/s/1016/ spanish after change http://git.reviewboard.kde.org/r/108328/s/1017/ Thanks, Xuetian Weng
Re: Review Request: fix 234407 - set minimum width for system settings icon view
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108328/ --- (Updated Jan. 10, 2013, 8:48 p.m.) Review request for kde-workspace, Ben Cooksley, Thomas Lübking, Chao Feng, and Yichao Yu. Changes --- add spanish screenshot before apply the patch. Description --- Set minimum width for system settings icon view item depending on font. Since KFileItemDelegate doesn't provides setMinimumSize, we make a sub-class that can have a minimumSize. (Maybe should be add to kdelibs in the future?) This fontHeight * 6 heuristic value works for all languages. CJK character is usually square (width = height) so this roughly means 6 CJK character, and 12 latin letter (height = width * 2), which will always look good. To fengchao, I'm sorry if you feel I steal your job.. but I can't stop myself since I think I can fix it in 10 minutes.. This addresses bug 234407. http://bugs.kde.org/show_bug.cgi?id=234407 Diffs - systemsettings/icons/CMakeLists.txt 0830dd7 systemsettings/icons/FileItemDelegate.h PRE-CREATION systemsettings/icons/FileItemDelegate.cpp PRE-CREATION systemsettings/icons/IconMode.cpp 37cfc4b Diff: http://git.reviewboard.kde.org/r/108328/diff/ Testing --- see screenshot. Screenshots (updated) --- english after change http://git.reviewboard.kde.org/r/108328/s/1014/ chinese after change http://git.reviewboard.kde.org/r/108328/s/1015/ chinese before change http://git.reviewboard.kde.org/r/108328/s/1016/ spanish after change http://git.reviewboard.kde.org/r/108328/s/1017/ spanish before change http://git.reviewboard.kde.org/r/108328/s/1023/ Thanks, Xuetian Weng
Re: Review Request: fix 234407 - set minimum width for system settings icon view
On Jan. 10, 2013, 11:47 p.m., Chao Feng wrote: Screenshot: chinese after change http://git.reviewboard.kde.org Others looks good, I still think here it looks strange. Increase to 7 characters? Yichao Yu wrote: Increase to 7 characters will only make 8 the number of characters in the text that looks strange. (and in fact it is 7 characters with my font settings.) This should be a different issue that may be solved by changing how word wrapping is done for short Chinese text in Qt instead of disabling word wrapping or increasing the limit to a magic value IMHO. See my last comment at https://git.reviewboard.kde.org/r/108285/ Picking a random value will actually not do if the goal is to prevent stupid wrapping of single glyphs. You'll have to reimplement the delegate painting and paint the rows omitting the cliprect and thus ignoring the maximum size for single remains. The risc with this approach is overlapping text (what can be migitated by a 2em margin between elements) and it needs to be accomplished by a reasonable minimum width (to prevent a 2 or even 3 char wide column) but picking a random static minimum size will fail on item a in language b for absolutely sure. - Thomas --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108328/#review25205 --- On Jan. 10, 2013, 8:48 p.m., Xuetian Weng wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108328/ --- (Updated Jan. 10, 2013, 8:48 p.m.) Review request for kde-workspace, Ben Cooksley, Thomas Lübking, Chao Feng, and Yichao Yu. Description --- Set minimum width for system settings icon view item depending on font. Since KFileItemDelegate doesn't provides setMinimumSize, we make a sub-class that can have a minimumSize. (Maybe should be add to kdelibs in the future?) This fontHeight * 6 heuristic value works for all languages. CJK character is usually square (width = height) so this roughly means 6 CJK character, and 12 latin letter (height = width * 2), which will always look good. To fengchao, I'm sorry if you feel I steal your job.. but I can't stop myself since I think I can fix it in 10 minutes.. This addresses bug 234407. http://bugs.kde.org/show_bug.cgi?id=234407 Diffs - systemsettings/icons/CMakeLists.txt 0830dd7 systemsettings/icons/FileItemDelegate.h PRE-CREATION systemsettings/icons/FileItemDelegate.cpp PRE-CREATION systemsettings/icons/IconMode.cpp 37cfc4b Diff: http://git.reviewboard.kde.org/r/108328/diff/ Testing --- see screenshot. Screenshots --- english after change http://git.reviewboard.kde.org/r/108328/s/1014/ chinese after change http://git.reviewboard.kde.org/r/108328/s/1015/ chinese before change http://git.reviewboard.kde.org/r/108328/s/1016/ spanish after change http://git.reviewboard.kde.org/r/108328/s/1017/ spanish before change http://git.reviewboard.kde.org/r/108328/s/1023/ Thanks, Xuetian Weng
Re: Review Request: Show larger icon in pager when the window rect is big enough
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108340/#review25211 --- This review has been submitted with commit 458757543a962c57f8e2836aa484e99d0061845f by Weng Xuetian on behalf of Yichao Yu to branch KDE/4.10. - Commit Hook On Jan. 11, 2013, 1:27 a.m., Yichao Yu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108340/ --- (Updated Jan. 11, 2013, 1:27 a.m.) Review request for kde-workspace and Luís Gabriel Lima. Description --- The size of window icon in the pager applet was always the same and is too small to if the applet is placed on the desktop. This patch increase the icon size if the window rectangle is big enough. Diffs - plasma/desktop/applets/pager/package/contents/ui/main.qml d3203b9 plasma/desktop/applets/pager/pager.cpp 70d1b55 Diff: http://git.reviewboard.kde.org/r/108340/diff/ Testing --- Compiled and see screen shots. Screenshots --- Before http://git.reviewboard.kde.org/r/108340/s/1024/ After http://git.reviewboard.kde.org/r/108340/s/1025/ Thanks, Yichao Yu
Re: New lockscreen
On Thursday 10 January 2013 19:37:57 Martin Sandsmark wrote: Hi! The new lock screen has some more or less serious regressions, and doesn't seem to be maintained by anyone in particular (one of the regression bugs filed against it is from november, and I don't really see anyone in particular commenting or fixing anything, it only got a handful of commits in december). FYI: I started investigating one bug and are working on it. It's not a trivial one, so after not fixing it instantly I moved it back, but I will work on it. For me KWin has higher priority. If you ask for maintainers: I consider Marco and me as the maintainers of the new screen locker, the old screen saver HACK (which you seem to be using) is something I consider as deprecated as legacy and it would be really awesome if someone who wants to use them would step up to maintain them. I voted for dropping them (see my blog for reasons). Cheers Martin signature.asc Description: This is a digitally signed message part.
