kdereview - qtcurve
QtCurve is now in kdereview with the intention of being in extragear/base
Re: Review Request 108308: 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: https://git.reviewboard.kde.org/r/108308/ --- (Updated Sept. 22, 2016, 8:35 p.m.) Status -- This change has been discarded. Review request for kdelibs, kwin, Plasma, Aaron J. Seigo, and Martin Gräßlin. Repository: kdelibs 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: https://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 126304: (re)enable building with -DQTC_QT5_ENABLE_KDE
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126304/#review90548 --- qt5/style/CMakeLists.txt (line 70) <https://git.reviewboard.kde.org/r/126304/#comment61891> trailing white space - Yichao Yu On 十二月 10, 2015, 12:12 p.m., René J.V. Bertin wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126304/ > --- > > (Updated 十二月 10, 2015, 12:12 p.m.) > > > Review request for KDE Frameworks, Qt KDE and Yichao Yu. > > > Repository: qtcurve > > > Description > --- > > As the title says, this restores the definition of QTC_QT5_ENABLE_KDE in the > CMake file when a KF5 build is done, and introduces the changes required for > that build to succeed. > > Except for removing references to the m_componentData member (already removed > from the class; deprecated type) I have not made any other changes to the > code. > > > Diffs > - > > qt5/CMakeLists.txt 837d9c2 > qt5/style/CMakeLists.txt 7f65f8c > qt5/style/qtcurve.cpp febcfcf > qt5/style/qtcurve_api.cpp 87a927f > qt5/style/qtcurve_p.h bfc7502 > qt5/style/qtcurve_primitive.cpp b5a3204 > > Diff: https://git.reviewboard.kde.org/r/126304/diff/ > > > Testing > --- > > On KUbuntu 14.04 and Mac OS X 10.9.5, both with Qt 5.5.1 and KF5 Frameworks > 5.16.0 installed under /opt/local > > > Thanks, > > René J.V. Bertin > >
Re: Review Request 126304: (re)enable building with -DQTC_QT5_ENABLE_KDE
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126304/#review90551 --- Ship it! LGTM other than the white space issue. Sorry for the delay. I'm checking my email for the review requests but maybe I'm not using the right filter or not subscribing to the right list. - Yichao Yu On 十二月 10, 2015, 12:12 p.m., René J.V. Bertin wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126304/ > --- > > (Updated 十二月 10, 2015, 12:12 p.m.) > > > Review request for KDE Frameworks, Qt KDE and Yichao Yu. > > > Repository: qtcurve > > > Description > --- > > As the title says, this restores the definition of QTC_QT5_ENABLE_KDE in the > CMake file when a KF5 build is done, and introduces the changes required for > that build to succeed. > > Except for removing references to the m_componentData member (already removed > from the class; deprecated type) I have not made any other changes to the > code. > > > Diffs > - > > qt5/CMakeLists.txt 837d9c2 > qt5/style/CMakeLists.txt 7f65f8c > qt5/style/qtcurve.cpp febcfcf > qt5/style/qtcurve_api.cpp 87a927f > qt5/style/qtcurve_p.h bfc7502 > qt5/style/qtcurve_primitive.cpp b5a3204 > > Diff: https://git.reviewboard.kde.org/r/126304/diff/ > > > Testing > --- > > On KUbuntu 14.04 and Mac OS X 10.9.5, both with Qt 5.5.1 and KF5 Frameworks > 5.16.0 installed under /opt/local > > > Thanks, > > René J.V. Bertin > >
Re: Review Request 126304: (re)enable building with -DQTC_QT5_ENABLE_KDE
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126304/#review90549 --- qt5/style/qtcurve_api.cpp (line 80) <https://git.reviewboard.kde.org/r/126304/#comment61892> Also here - Yichao Yu On 十二月 10, 2015, 12:12 p.m., René J.V. Bertin wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126304/ > --- > > (Updated 十二月 10, 2015, 12:12 p.m.) > > > Review request for KDE Frameworks, Qt KDE and Yichao Yu. > > > Repository: qtcurve > > > Description > --- > > As the title says, this restores the definition of QTC_QT5_ENABLE_KDE in the > CMake file when a KF5 build is done, and introduces the changes required for > that build to succeed. > > Except for removing references to the m_componentData member (already removed > from the class; deprecated type) I have not made any other changes to the > code. > > > Diffs > - > > qt5/CMakeLists.txt 837d9c2 > qt5/style/CMakeLists.txt 7f65f8c > qt5/style/qtcurve.cpp febcfcf > qt5/style/qtcurve_api.cpp 87a927f > qt5/style/qtcurve_p.h bfc7502 > qt5/style/qtcurve_primitive.cpp b5a3204 > > Diff: https://git.reviewboard.kde.org/r/126304/diff/ > > > Testing > --- > > On KUbuntu 14.04 and Mac OS X 10.9.5, both with Qt 5.5.1 and KF5 Frameworks > 5.16.0 installed under /opt/local > > > Thanks, > > René J.V. Bertin > >
Re: Review Request 121390: make Qt5 theme build on Linux again
engine present doesn't mean that indidual KDE applications or plugins can no longer decide to support only a subset to be set at build time. *) No issue either with Unix but not OS X - that's what I came up with for lack of something better. Turns out Yichao has his own alternative to HAVE_X11, I'll see if I can make do with that. *) or else I'll start making a ruckus to have kwin and more Plasma goodies on OS X!! ;) Martin Gräßlin wrote: Yes, it's not about compile time checks, it's about introducing runtime checks as Thomas and I wrote ;-) René J.V. Bertin wrote: Actually, Thomas wrote The compile time checks have no implication on the runtime. Surely a typo, but those can have devastating consequences around code ;) René J.V. Bertin wrote: (published too fast again) Actually, that blog post of yours also starts out by talking exclusively about compile-time checks for about 2/3 of its length. It's only after the screenshot that it becomes clear you actually use the compile-time check to include a runtime-check or not. A casual reader might be tempted to stop reading early, thinking he got the message ... And I can't stop thinking something that has been stamped into me: ifs are expensive. Guess that shows my age ... Thomas Lübking wrote: That's not a typo. Meaning distorting partial quote. I wrote: The compile time checks have no implication on the runtime *environment*. Ifs are expensive might be stamped into your mind and/or true, but they're completely inavoidable in this context. Just that X11 was available at runtime does NOT (no more w/ Qt5) mean that it's available at runtime. = Keep the branching out of hot loops (as always) ;-) René J.V. Bertin wrote: yes, I know I didn't copy the last word of your statement. That doesn't change the fact that your 2nd word was *compile* instead of *run*, in a context where you (at least) seemed to be saying that I apparently claimed that those (= compile time) checks had an impact on runtime performance. Anyway, yes, I understood perfectly well that X11 might not be available at runtime while it was when compiling, and that an application trying to do X11 calls will exit with an error when trying to connect to an inexisting X11 server. (Or crash if X11 was actually uninstalled ... but it would take other runtime checks to protect against that, and frankly that'd just be crazy.) Ifs are expensive might be stamped into your mind and/or true, but they're completely inavoidable in this context. Not true, see my remarks about using function pointers above. Not that that would be particularly clever and less expensive when X11 is the only platform that provides a certain functionality ... :) (I do seem to recall that using function pointers instead of normal functions was hardly more expensive on x86) Sorry somehow my filter missed this review request and I've just seen it today... To answer Martin's concern, I totally agree and it's in my mind the first time I added X11 support back to the Qt5 version. The X11 related stuff in `libqtcurve-utils` have also always had that check. All X11 related functions are guarded by both a compile time check (e.g. if libxcb/X11 is not found or somehow the user simply don't want to link to them...) and a runtime check (i.e. the X11 related functions are no-ops if X11 is not initialized first at runtime). Now (AFAIK) the compile failure on OSX seems to be related to some sturture name conflict (or whatever it is that causes a forward declaration of `Display` not working...). The real issue is already addressed in another review request and it is not necessary to disable calls to X11 related functions (which might be no-ops) on OSX anymore. In any case, the issue related to this request should already be resolved now and the status is also monitored on build.kde.org (and AFAIK both Qt4 and Qt5 versions build successfully now). I think this review request can be discarded. - Yichao --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121390/#review71553 --- On 十二月 8, 2014, 4:59 p.m., René J.V. Bertin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121390/ --- (Updated 十二月 8, 2014, 4:59 p.m.) Review request for KDE Frameworks, Qt KDE and Yichao Yu. Repository: qtcurve Description --- Yesterday's patches for OS X building broke the build of the Qt5 parts on Linux (and other Unix/X11 platforms). I had presumed that Q_WS_X11 would be defined
Re: Review Request 121390: make Qt5 theme build on Linux again
engine present doesn't mean that indidual KDE applications or plugins can no longer decide to support only a subset to be set at build time. *) No issue either with Unix but not OS X - that's what I came up with for lack of something better. Turns out Yichao has his own alternative to HAVE_X11, I'll see if I can make do with that. *) or else I'll start making a ruckus to have kwin and more Plasma goodies on OS X!! ;) Martin Gräßlin wrote: Yes, it's not about compile time checks, it's about introducing runtime checks as Thomas and I wrote ;-) René J.V. Bertin wrote: Actually, Thomas wrote The compile time checks have no implication on the runtime. Surely a typo, but those can have devastating consequences around code ;) René J.V. Bertin wrote: (published too fast again) Actually, that blog post of yours also starts out by talking exclusively about compile-time checks for about 2/3 of its length. It's only after the screenshot that it becomes clear you actually use the compile-time check to include a runtime-check or not. A casual reader might be tempted to stop reading early, thinking he got the message ... And I can't stop thinking something that has been stamped into me: ifs are expensive. Guess that shows my age ... Thomas Lübking wrote: That's not a typo. Meaning distorting partial quote. I wrote: The compile time checks have no implication on the runtime *environment*. Ifs are expensive might be stamped into your mind and/or true, but they're completely inavoidable in this context. Just that X11 was available at runtime does NOT (no more w/ Qt5) mean that it's available at runtime. = Keep the branching out of hot loops (as always) ;-) René J.V. Bertin wrote: yes, I know I didn't copy the last word of your statement. That doesn't change the fact that your 2nd word was *compile* instead of *run*, in a context where you (at least) seemed to be saying that I apparently claimed that those (= compile time) checks had an impact on runtime performance. Anyway, yes, I understood perfectly well that X11 might not be available at runtime while it was when compiling, and that an application trying to do X11 calls will exit with an error when trying to connect to an inexisting X11 server. (Or crash if X11 was actually uninstalled ... but it would take other runtime checks to protect against that, and frankly that'd just be crazy.) Ifs are expensive might be stamped into your mind and/or true, but they're completely inavoidable in this context. Not true, see my remarks about using function pointers above. Not that that would be particularly clever and less expensive when X11 is the only platform that provides a certain functionality ... :) (I do seem to recall that using function pointers instead of normal functions was hardly more expensive on x86) Yichao Yu wrote: Sorry somehow my filter missed this review request and I've just seen it today... To answer Martin's concern, I totally agree and it's in my mind the first time I added X11 support back to the Qt5 version. The X11 related stuff in `libqtcurve-utils` have also always had that check. All X11 related functions are guarded by both a compile time check (e.g. if libxcb/X11 is not found or somehow the user simply don't want to link to them...) and a runtime check (i.e. the X11 related functions are no-ops if X11 is not initialized first at runtime). Now (AFAIK) the compile failure on OSX seems to be related to some sturture name conflict (or whatever it is that causes a forward declaration of `Display` not working...). The real issue is already addressed in another review request and it is not necessary to disable calls to X11 related functions (which might be no-ops) on OSX anymore. In any case, the issue related to this request should already be resolved now and the status is also monitored on build.kde.org (and AFAIK both Qt4 and Qt5 versions build successfully now). I think this review request can be discarded. Marko Käning wrote: Just for the record, QtCurve currently fails to build on OSX/CI: /Users/marko/WC/KDECI-builds/kf5-qt5/qtcurve/qt5/config/qtcurveconfig.cpp:1085: Warning: Macro argument mismatch. In file included from /Users/marko/WC/KDECI-builds/kf5-qt5/qtcurve/lib/utils/dirs.cpp:22: /Users/marko/WC/KDECI-builds/kf5-qt5/qtcurve/lib/utils/dirs.h:68:1: error: implicit instantiation of undefined template 'std::__1::basic_stringchar, std::__1::char_traitschar, std::__1:: allocatorchar ' getConfFile(const std::string file) ^ Shall I send the full build log of this failure to you via PM? Marko Käning wrote: For completeness: I haven't THIS RR applied to my OSX/CI system as of now
Re: Review Request 121390: make Qt5 theme build on Linux again
engine present doesn't mean that indidual KDE applications or plugins can no longer decide to support only a subset to be set at build time. *) No issue either with Unix but not OS X - that's what I came up with for lack of something better. Turns out Yichao has his own alternative to HAVE_X11, I'll see if I can make do with that. *) or else I'll start making a ruckus to have kwin and more Plasma goodies on OS X!! ;) Martin Gräßlin wrote: Yes, it's not about compile time checks, it's about introducing runtime checks as Thomas and I wrote ;-) René J.V. Bertin wrote: Actually, Thomas wrote The compile time checks have no implication on the runtime. Surely a typo, but those can have devastating consequences around code ;) René J.V. Bertin wrote: (published too fast again) Actually, that blog post of yours also starts out by talking exclusively about compile-time checks for about 2/3 of its length. It's only after the screenshot that it becomes clear you actually use the compile-time check to include a runtime-check or not. A casual reader might be tempted to stop reading early, thinking he got the message ... And I can't stop thinking something that has been stamped into me: ifs are expensive. Guess that shows my age ... Thomas Lübking wrote: That's not a typo. Meaning distorting partial quote. I wrote: The compile time checks have no implication on the runtime *environment*. Ifs are expensive might be stamped into your mind and/or true, but they're completely inavoidable in this context. Just that X11 was available at runtime does NOT (no more w/ Qt5) mean that it's available at runtime. = Keep the branching out of hot loops (as always) ;-) René J.V. Bertin wrote: yes, I know I didn't copy the last word of your statement. That doesn't change the fact that your 2nd word was *compile* instead of *run*, in a context where you (at least) seemed to be saying that I apparently claimed that those (= compile time) checks had an impact on runtime performance. Anyway, yes, I understood perfectly well that X11 might not be available at runtime while it was when compiling, and that an application trying to do X11 calls will exit with an error when trying to connect to an inexisting X11 server. (Or crash if X11 was actually uninstalled ... but it would take other runtime checks to protect against that, and frankly that'd just be crazy.) Ifs are expensive might be stamped into your mind and/or true, but they're completely inavoidable in this context. Not true, see my remarks about using function pointers above. Not that that would be particularly clever and less expensive when X11 is the only platform that provides a certain functionality ... :) (I do seem to recall that using function pointers instead of normal functions was hardly more expensive on x86) Yichao Yu wrote: Sorry somehow my filter missed this review request and I've just seen it today... To answer Martin's concern, I totally agree and it's in my mind the first time I added X11 support back to the Qt5 version. The X11 related stuff in `libqtcurve-utils` have also always had that check. All X11 related functions are guarded by both a compile time check (e.g. if libxcb/X11 is not found or somehow the user simply don't want to link to them...) and a runtime check (i.e. the X11 related functions are no-ops if X11 is not initialized first at runtime). Now (AFAIK) the compile failure on OSX seems to be related to some sturture name conflict (or whatever it is that causes a forward declaration of `Display` not working...). The real issue is already addressed in another review request and it is not necessary to disable calls to X11 related functions (which might be no-ops) on OSX anymore. In any case, the issue related to this request should already be resolved now and the status is also monitored on build.kde.org (and AFAIK both Qt4 and Qt5 versions build successfully now). I think this review request can be discarded. Marko Käning wrote: Just for the record, QtCurve currently fails to build on OSX/CI: /Users/marko/WC/KDECI-builds/kf5-qt5/qtcurve/qt5/config/qtcurveconfig.cpp:1085: Warning: Macro argument mismatch. In file included from /Users/marko/WC/KDECI-builds/kf5-qt5/qtcurve/lib/utils/dirs.cpp:22: /Users/marko/WC/KDECI-builds/kf5-qt5/qtcurve/lib/utils/dirs.h:68:1: error: implicit instantiation of undefined template 'std::__1::basic_stringchar, std::__1::char_traitschar, std::__1:: allocatorchar ' getConfFile(const std::string file) ^ Shall I send the full build log of this failure to you via PM? Marko Käning wrote: For completeness: I haven't THIS RR applied to my OSX/CI system as of now
Re: Review Request 113969: Do not assume every items have the same height
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/113969/ --- (Updated May 8, 2014, 10:45 p.m.) Status -- This change has been marked as submitted. Review request for kdelibs, David Faure, Rafael Fernández López, and Michael Pyne. Bugs: 309780 http://bugs.kde.org/show_bug.cgi?id=309780 Repository: kdelibs Description --- 1. the offset addjust in `KCategorizedView::indexAt` was a no-op (it operates on a temporary variable and is not needed). 2. KCategorizedView::indexAt (effectively) assumes all items has the same height when doing bsearch and therefore failed to handle some cases when the text takes multiple lines as shown in the bug report. This patch removes the no-op and add special check for items in the same row on the left (or on the right for RightToLeft layout) in order to determine which way the bsearch should go. Diffs - kdeui/itemviews/kcategorizedview.cpp be811aa Diff: https://git.reviewboard.kde.org/r/113969/diff/ Testing --- Compiles and fixes the problem. Tested with systemsettings in the following conditions: 1. single row in each category. 2. multiple rows in each category. 3. scrollbar not at the top. Thanks, Yichao Yu
Re: Review Request 113969: Do not assume every items have the same height
On April 6, 2014, 7:48 a.m., David Faure wrote: kdeui/itemviews/kcategorizedview.cpp, line 795 https://git.reviewboard.kde.org/r/113969/diff/4/?file=215105#file215105line795 Coding style comment: some code paths use break (which results in return QModelIndex() at the end) and some other code paths (e.g. line 776) do return QModelIndex() directly. Any reason for the inconsistency? The break at this line is not inconsistent with the `return QModelIndex()` at line 776 (line 764 in the new patch) because that line is inside another for loop. It is indeed not consistent with the old code at line 750 (line 738 in the new patch) which I didn't noticed before. I've changed this in the new patch to be consistent with the old style. - Yichao --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/113969/#review55062 --- On April 22, 2014, 6:13 a.m., Yichao Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/113969/ --- (Updated April 22, 2014, 6:13 a.m.) Review request for kdelibs, David Faure, Rafael Fernández López, and Michael Pyne. Bugs: 309780 http://bugs.kde.org/show_bug.cgi?id=309780 Repository: kdelibs Description --- 1. the offset addjust in `KCategorizedView::indexAt` was a no-op (it operates on a temporary variable and is not needed). 2. KCategorizedView::indexAt (effectively) assumes all items has the same height when doing bsearch and therefore failed to handle some cases when the text takes multiple lines as shown in the bug report. This patch removes the no-op and add special check for items in the same row on the left (or on the right for RightToLeft layout) in order to determine which way the bsearch should go. Diffs - kdeui/itemviews/kcategorizedview.cpp be811aa Diff: https://git.reviewboard.kde.org/r/113969/diff/ Testing --- Compiles and fixes the problem. Tested with systemsettings in the following conditions: 1. single row in each category. 2. multiple rows in each category. 3. scrollbar not at the top. Thanks, Yichao Yu
Re: Review Request 113969: Do not assume every items have the same height
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/113969/ --- (Updated April 22, 2014, 6:13 a.m.) Review request for kdelibs, David Faure, Rafael Fernández López, and Michael Pyne. Bugs: 309780 http://bugs.kde.org/show_bug.cgi?id=309780 Repository: kdelibs Description --- 1. the offset addjust in `KCategorizedView::indexAt` was a no-op (it operates on a temporary variable and is not needed). 2. KCategorizedView::indexAt (effectively) assumes all items has the same height when doing bsearch and therefore failed to handle some cases when the text takes multiple lines as shown in the bug report. This patch removes the no-op and add special check for items in the same row on the left (or on the right for RightToLeft layout) in order to determine which way the bsearch should go. Diffs (updated) - kdeui/itemviews/kcategorizedview.cpp be811aa Diff: https://git.reviewboard.kde.org/r/113969/diff/ Testing --- Compiles and fixes the problem. Tested with systemsettings in the following conditions: 1. single row in each category. 2. multiple rows in each category. 3. scrollbar not at the top. Thanks, Yichao Yu
Re: Review Request 113969: Do not assume every items have the same height
On Nov. 20, 2013, 5:27 p.m., Christoph Feck wrote: I love people who report bugs, and one year later come up with a patch :P Anyway, nice analysis, and this probably also fixes bug 290971, but have not tested it yet. Yichao Yu wrote: Unfortunately I think I can still reproduce those problems. (I noticed the problem when testing my patch and it's good to know it is not caused by me (not likely anyway...)) There are indeed other places in the code that suffers from the same problem (blockHeight and intersectingIndexesWithRect looks suspicious at least) but I am not sure and I think it is a better idea to fix them one by one... Ping. Can someone review this? I'm not completely sure if this is the best solution for the bug but I believe it is the right place to fix it and I'm willing to improve the patch if someone can provide some suggestions. - Yichao --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113969/#review44071 --- On Nov. 20, 2013, 4:47 p.m., Yichao Yu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113969/ --- (Updated Nov. 20, 2013, 4:47 p.m.) Review request for kdelibs, David Faure, Rafael Fernández López, and Michael Pyne. Bugs: 309780 http://bugs.kde.org/show_bug.cgi?id=309780 Repository: kdelibs Description --- 1. the offset addjust in `KCategorizedView::indexAt` was a no-op (it operates on a temporary variable and is not needed). 2. KCategorizedView::indexAt (effectively) assumes all items has the same height when doing bsearch and therefore failed to handle some cases when the text takes multiple lines as shown in the bug report. This patch removes the no-op and add special check for items in the same row on the left (or on the right for RightToLeft layout) in order to determine which way the bsearch should go. Diffs - kdeui/itemviews/kcategorizedview.cpp 010bcbc Diff: http://git.reviewboard.kde.org/r/113969/diff/ Testing --- Compiles and fixes the problem. Tested with systemsettings in the following conditions: 1. single row in each category. 2. multiple rows in each category. 3. scrollbar not at the top. Thanks, Yichao Yu
Re: Review Request 114219: Do not encode QString to QByteArray and cast it back to QString. This causes problem when there are Unicode characters in ${HOME}
On Dec. 1, 2013, 3:47 a.m., David Faure wrote: Yes, clearly correct. For your question about branches: commit in the stable branch and merge upwards (or ask the module maintainers to merge upwards). Thank you for the review. I don't have a git account yet (will apply soon) so could you please push that for me? I would also be very nice if you can cherry-pick this patch as well as these two[1][2] to the corresponding framework/master branches. (All of them should apply straightforwardly with difference only in the context or file paths). Thank you. [1] https://git.reviewboard.kde.org/r/113939/ [2] https://git.reviewboard.kde.org/r/113985/ - Yichao --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/114219/#review44926 --- On Nov. 30, 2013, 2:55 p.m., Yichao Yu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/114219/ --- (Updated Nov. 30, 2013, 2:55 p.m.) Review request for kde-workspace, David Faure, Martin Gräßlin, and Hugo Pereira Da Costa. Bugs: 327919 http://bugs.kde.org/show_bug.cgi?id=327919 Repository: kde-workspace Description --- list.join already returns a QString and there is no need to encode it and cast back to QString again P.S. for a patch that applies to both KDE4 and KF5(master for kde-workspace, frameworks for kdelibs?) How should I submit review request? Should I add both in branch or submit two review request? (But often the patch cannot apply directly due to context or file path changes). Diffs - kcontrol/krdb/krdb.cpp 92d84e9 Diff: http://git.reviewboard.kde.org/r/114219/diff/ Testing --- Compiles. Fixes the problem here. Also fixes the problem for the reporter. Thanks, Yichao Yu
Re: Review Request 114219: Do not encode QString to QByteArray and cast it back to QString. This causes problem when there are Unicode characters in ${HOME}
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/114219/ --- (Updated Dec. 2, 2013, 7:27 a.m.) Status -- This change has been marked as submitted. Review request for kde-workspace, David Faure, Martin Gräßlin, and Hugo Pereira Da Costa. Bugs: 327919 http://bugs.kde.org/show_bug.cgi?id=327919 Repository: kde-workspace Description --- list.join already returns a QString and there is no need to encode it and cast back to QString again P.S. for a patch that applies to both KDE4 and KF5(master for kde-workspace, frameworks for kdelibs?) How should I submit review request? Should I add both in branch or submit two review request? (But often the patch cannot apply directly due to context or file path changes). Diffs - kcontrol/krdb/krdb.cpp 92d84e9 Diff: http://git.reviewboard.kde.org/r/114219/diff/ Testing --- Compiles. Fixes the problem here. Also fixes the problem for the reporter. Thanks, Yichao Yu
Re: Review Request 114219: Do not encode QString to QByteArray and cast it back to QString. This causes problem when there are Unicode characters in ${HOME}
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/114219/ --- (Updated Nov. 30, 2013, 2:55 p.m.) Review request for kde-workspace, David Faure, Martin Gräßlin, and Hugo Pereira Da Costa. Bugs: 327919 http://bugs.kde.org/show_bug.cgi?id=327919 Repository: kde-workspace Description --- list.join already returns a QString and there is no need to encode it and cast back to QString again P.S. for a patch that applies to both KDE4 and KF5(master for kde-workspace, frameworks for kdelibs?) How should I submit review request? Should I add both in branch or submit two review request? (But often the patch cannot apply directly due to context or file path changes). Diffs - kcontrol/krdb/krdb.cpp 92d84e9 Diff: http://git.reviewboard.kde.org/r/114219/diff/ Testing (updated) --- Compiles. Fixes the problem here. Also fixes the problem for the reporter. Thanks, Yichao Yu
Review Request 114219: Do not encode QString to QByteArray and cast it back to QString. This causes problem when there are Unicode characters in ${HOME}
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/114219/ --- Review request for kde-workspace, David Faure, Martin Gräßlin, and Hugo Pereira Da Costa. Bugs: 327919 http://bugs.kde.org/show_bug.cgi?id=327919 Repository: kde-workspace Description --- list.join already returns a QString and there is no need to encode it and cast back to QString again P.S. for a patch that applies to both KDE4 and KF5(master for kde-workspace, frameworks for kdelibs?) How should I submit review request? Should I add both in branch or submit two review request? (But often the patch cannot apply directly due to context or file path changes). Diffs - kcontrol/krdb/krdb.cpp 92d84e9 Diff: http://git.reviewboard.kde.org/r/114219/diff/ Testing --- Compiles. Waiting for bug reporter's test. Thanks, Yichao Yu
Re: Review Request 114219: Do not encode QString to QByteArray and cast it back to QString. This causes problem when there are Unicode characters in ${HOME}
On Nov. 29, 2013, 7:45 p.m., Thomas Lübking wrote:
kcontrol/krdb/krdb.cpp, line 102
http://git.reviewboard.kde.org/r/114219/diff/1/?file=221836#file221836line102
QFile::encodeName() seems equal to QString::toLocal8Bit(),
::decodeName() to ::fromLocal8Bit()
I don't think one can just drop one of them and whether transcoding is
required probably depends on what is done to the string interim.
If at all KToolInvocation::klauncher()-setLaunchEnv() would perform
a second decode, so it probaly depends on what that does.
Was locale charmap determined by the reporter in the bug?
---
Secret world domination plan:
--
#1: classified
#2: classified
#3: force ASCII as global standard
#4: classified
#5: classified
#6: classified
#7: classified
#8: classified
#9: classified
#a: classified
Yichao Yu wrote:
encodeName/toLocal8Bit is used to encode a unicode string to a
c-string/byte-array representation and decodeName/fromLocal8Bit does the
reverse.
The proper decoding is already done in QFile::decodeName above and the
QString already has the right unicode string in it.
Basically, QString is not a wrapper of arbitrary c-string/byte-array,
rather a wrapper of a unicode string so whatever done to a QString before or
after should assume it is a valid unicode string and is independent of what
encoding (utf8 in the case of dbus) is needed afterward.
Encode to a byte array and cast it back can only cause wrong encoding in
the second conversion and will not affect what is done in setLaunchEnv.
Yichao Yu wrote:
Or in another word QString has no encoding (well, by which I mean the
internal encoding is trasparent to the user), only byte array and c-string
has encoding.
Thomas Lübking wrote:
QString(QByteArray) according to the API doc actually differs between Qt4
5 (fromAscii - fromUtf8) but an encoding should not happen nevertheless
because:
282 void KLauncher::setLaunchEnv(const QString name, const QString
value)
283 {
284 #ifndef USE_KPROCESS_FOR_KIOSLAVES
285klauncher_header request_header;
286QByteArray requestData;
287
requestData.append(name.toLocal8Bit()).append('\0').append(value.toLocal8Bit()).append('\0');
Also QString(QByteArray) is obvisouly problematic by itself for the
apparent 4/5 incompatibility.
Yichao Yu wrote:
I guess you can also put it in this this way (setLaunchEnv have
toLocal8Bit inside) although I still think the simplest way is to remember
QString -- encode -- QByteArray, QByteArray -- decode -- QString and always
to the necessary explicit conversion.
That's why I hate hate hate this constructor. (and I've already fixed 3-4
bugs in KDE due to this constructor.) It might actually be helpful to compile
KDE with it commented out and replace everything with explicit conversion.
Yichao Yu wrote:
I guess you can also put it in this this way (setLaunchEnv have
toLocal8Bit inside) although I still think the simplest way is to remember
QString -- encode -- QByteArray, QByteArray -- decode -- QString and always
to the necessary explicit conversion.
That's why I hate hate hate this constructor. (and I've already fixed 3-4
bugs in KDE due to this constructor.) It might actually be helpful to compile
KDE with it commented out and replace everything with explicit conversion.
Yichao Yu wrote:
I guess you can also put it in this this way (setLaunchEnv have
toLocal8Bit inside) although I still think the simplest way is to remember
QString -- encode -- QByteArray, QByteArray -- decode -- QString and always
to the necessary explicit conversion.
That's why I hate hate hate this constructor. (and I've already fixed 3-4
bugs in KDE due to this constructor.) It might actually be helpful to compile
KDE with it commented out and replace everything with explicit conversion.
ahh sth wrong with my network sorry for the duplicated post...
- Yichao
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/114219/#review44857
---
On Nov. 29, 2013, 7:26 p.m., Yichao Yu wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/114219/
---
(Updated Nov. 29, 2013, 7:26 p.m.)
Review request for kde-workspace, David Faure, Martin Gräßlin, and Hugo
Pereira Da Costa.
Bugs: 327919
http://bugs.kde.org/show_bug.cgi?id=327919
Repository: kde-workspace
Description
Re: Review Request 113985: Remove no-ops in KCategorizedView
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113985/ --- (Updated Nov. 27, 2013, 9:52 a.m.) Status -- This change has been marked as submitted. Review request for kdelibs, David Faure, Rafael Fernández López, and Michael Pyne. Repository: kdelibs Description --- Remove operations on temporary returned objects. These are no-ops and are not needed since they are already done in visualRect. Diffs - kdeui/itemviews/kcategorizedview.cpp 010bcbc Diff: http://git.reviewboard.kde.org/r/113985/diff/ Testing --- It compiles. (It compiles when just adding const to the QRect, proving they are indeed no-op's). Thanks, Yichao Yu
Re: Review Request 113985: Remove no-ops in KCategorizedView
On Nov. 25, 2013, 1:35 p.m., David Faure wrote: Ship It! Thanks for the review. However, I don't have a git account yet and I cannot find others to push it for me this time. Could you please push it for me? - Yichao --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113985/#review44451 --- On Nov. 21, 2013, 8:34 p.m., Yichao Yu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113985/ --- (Updated Nov. 21, 2013, 8:34 p.m.) Review request for kdelibs, David Faure, Rafael Fernández López, and Michael Pyne. Repository: kdelibs Description --- Remove operations on temporary returned objects. These are no-ops and are not needed since they are already done in visualRect. Diffs - kdeui/itemviews/kcategorizedview.cpp 010bcbc Diff: http://git.reviewboard.kde.org/r/113985/diff/ Testing --- It compiles. (It compiles when just adding const to the QRect, proving they are indeed no-op's). Thanks, Yichao Yu
Re: Review Request 113969: Do not assume every items have the same height
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113969/ --- (Updated Nov. 20, 2013, 12:24 p.m.) Review request for kdelibs, David Faure, Rafael Fernández López, and Michael Pyne. Bugs: 309780 http://bugs.kde.org/show_bug.cgi?id=309780 Repository: kdelibs Description (updated) --- 1. the offset addjust in `KCategorizedView::indexAt` was a no-op (it operates on a temporary variable and is not needed). 2. KCategorizedView::indexAt (effectively) assumes all items has the same height when doing bsearch and therefore failed to handle some cases when the text takes multiple lines as shown in the bug report. This patch removes the no-op and add special check for items in the same row on the left (or on the right for RightToLeft layout) in order to determine which way the bsearch should go. Diffs - kdeui/itemviews/kcategorizedview.cpp 010bcbc Diff: http://git.reviewboard.kde.org/r/113969/diff/ Testing --- Compiles and fixes the problem. Tested with systemsettings in the following conditions: 1. single row in each category. 2. multiple rows in each category. 3. scrollbar not at the top. Thanks, Yichao Yu
Review Request 113969: Do not assume every items have the same height
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113969/ --- Review request for kdelibs, David Faure, Rafael Fernández López, and Michael Pyne. Bugs: 309780 http://bugs.kde.org/show_bug.cgi?id=309780 Repository: kdelibs Description --- 1. the offset addjust in `KCategorizedView::indexAt` was a no-op (it operates on a temporary variable and is not needed). 2. KCategorizedView::indexAt (effectively) assumes all items has the same height when doing bsearch and therefore failed to handle some cases when the text takes multiple lines as shown in the bug report. This patch removes the no-op and add special check for items in the same role on the left (or on the right for RightToLeft layout) in order to determine which way the bsearch should go. Diffs - kdeui/itemviews/kcategorizedview.cpp 010bcbc Diff: http://git.reviewboard.kde.org/r/113969/diff/ Testing --- Compiles and fixes the problem. Tested with systemsettings in the following conditions: 1. single row in each category. 2. multiple rows in each category. 3. scrollbar not at the top. Thanks, Yichao Yu
Review Request 113985: Remove no-ops in KCategorizedView
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113985/ --- Review request for kdelibs, David Faure, Rafael Fernández López, and Michael Pyne. Repository: kdelibs Description --- Remove operations on temporary returned objects. Diffs - kdeui/itemviews/kcategorizedview.cpp 010bcbc Diff: http://git.reviewboard.kde.org/r/113985/diff/ Testing --- It compiles. (It compiles when just adding const to the QRect, proving they are indeed no-op's). Thanks, Yichao Yu
Re: Review Request 113969: Do not assume every items have the same height
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113969/ --- (Updated Nov. 20, 2013, 4:46 p.m.) Review request for kdelibs, David Faure, Rafael Fernández López, and Michael Pyne. Bugs: 309780 http://bugs.kde.org/show_bug.cgi?id=309780 Repository: kdelibs Description --- 1. the offset addjust in `KCategorizedView::indexAt` was a no-op (it operates on a temporary variable and is not needed). 2. KCategorizedView::indexAt (effectively) assumes all items has the same height when doing bsearch and therefore failed to handle some cases when the text takes multiple lines as shown in the bug report. This patch removes the no-op and add special check for items in the same row on the left (or on the right for RightToLeft layout) in order to determine which way the bsearch should go. Diffs (updated) - kdeui/itemviews/kcategorizedview.cpp 010bcbc Diff: http://git.reviewboard.kde.org/r/113969/diff/ Testing --- Compiles and fixes the problem. Tested with systemsettings in the following conditions: 1. single row in each category. 2. multiple rows in each category. 3. scrollbar not at the top. Thanks, Yichao Yu
Re: Review Request 113969: Do not assume every items have the same height
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113969/ --- (Updated Nov. 20, 2013, 4:47 p.m.) Review request for kdelibs, David Faure, Rafael Fernández López, and Michael Pyne. Bugs: 309780 http://bugs.kde.org/show_bug.cgi?id=309780 Repository: kdelibs Description --- 1. the offset addjust in `KCategorizedView::indexAt` was a no-op (it operates on a temporary variable and is not needed). 2. KCategorizedView::indexAt (effectively) assumes all items has the same height when doing bsearch and therefore failed to handle some cases when the text takes multiple lines as shown in the bug report. This patch removes the no-op and add special check for items in the same row on the left (or on the right for RightToLeft layout) in order to determine which way the bsearch should go. Diffs (updated) - kdeui/itemviews/kcategorizedview.cpp 010bcbc Diff: http://git.reviewboard.kde.org/r/113969/diff/ Testing --- Compiles and fixes the problem. Tested with systemsettings in the following conditions: 1. single row in each category. 2. multiple rows in each category. 3. scrollbar not at the top. Thanks, Yichao Yu
Re: Review Request 113969: Do not assume every items have the same height
On Nov. 20, 2013, 5:27 p.m., Christoph Feck wrote: I love people who report bugs, and one year later come up with a patch :P Anyway, nice analysis, and this probably also fixes bug 290971, but have not tested it yet. Unfortunately I think I can still reproduce those problems. (I noticed the problem when testing my patch and it's good to know it is not caused by me (not likely anyway...)) There are indeed other places in the code that suffers from the same problem (blockHeight and intersectingIndexesWithRect looks suspicious at least) but I am not sure and I think it is a better idea to fix them one by one... - Yichao --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113969/#review44071 --- On Nov. 20, 2013, 4:47 p.m., Yichao Yu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113969/ --- (Updated Nov. 20, 2013, 4:47 p.m.) Review request for kdelibs, David Faure, Rafael Fernández López, and Michael Pyne. Bugs: 309780 http://bugs.kde.org/show_bug.cgi?id=309780 Repository: kdelibs Description --- 1. the offset addjust in `KCategorizedView::indexAt` was a no-op (it operates on a temporary variable and is not needed). 2. KCategorizedView::indexAt (effectively) assumes all items has the same height when doing bsearch and therefore failed to handle some cases when the text takes multiple lines as shown in the bug report. This patch removes the no-op and add special check for items in the same row on the left (or on the right for RightToLeft layout) in order to determine which way the bsearch should go. Diffs - kdeui/itemviews/kcategorizedview.cpp 010bcbc Diff: http://git.reviewboard.kde.org/r/113969/diff/ Testing --- Compiles and fixes the problem. Tested with systemsettings in the following conditions: 1. single row in each category. 2. multiple rows in each category. 3. scrollbar not at the top. Thanks, Yichao Yu
Re: Review Request 109133: Allow async KAuth Action with helper (and disallow async without helper)
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109133/ --- (Updated Feb. 25, 2013, 11:25 p.m.) Review request for kdelibs, Aaron J. Seigo, Dario Freddi, David Faure, and Kevin Ottens. Description --- It is async action without helper, instead of with helper, that doesn't make much sense. http://lists.kde.org/?l=kde-develm=135274807824550w=2 This addresses bug 310149. http://bugs.kde.org/show_bug.cgi?id=310149 Diffs - kdecore/auth/kauthaction.cpp 181547f Diff: http://git.reviewboard.kde.org/r/109133/diff/ Testing --- Following pykde4 code doesn't report error and the action is executed successfully (with a valid action_id, helper_id and arguments). action = KAuth.Action(action_id) action.setHelperID(helper_id) action.setArguments(args) if hasattr(callback, __call__): # the new-style signal connecting somehow doesn't work here.. QObject.connect(action.watcher(), SIGNAL(actionPerformed(const KAuth::ActionReply)), lambda reply: callback(reply.succeeded())) action.setExecutesAsync(True) reply = action.execute() if reply.failed(): return False return True Thanks, Yichao Yu
Review Request 109133: Allow async KAuth Action with helper (and disallow async without helper)
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109133/ --- Review request for kdelibs and Dario Freddi. Description --- It is async action without helper, instead of with helper, that doesn't make much sense. http://lists.kde.org/?l=kde-develm=135274807824550w=2 This addresses bug 310149. http://bugs.kde.org/show_bug.cgi?id=310149 Diffs - kdecore/auth/kauthaction.cpp 181547f Diff: http://git.reviewboard.kde.org/r/109133/diff/ Testing --- Following pykde4 code doesn't report error and the action is executed successfully (with a valid action_id, helper_id and arguments). action = KAuth.Action(action_id) action.setHelperID(helper_id) action.setArguments(args) if hasattr(callback, __call__): # the new-style signal connecting somehow doesn't work here.. QObject.connect(action.watcher(), SIGNAL(actionPerformed(const KAuth::ActionReply)), lambda reply: callback(reply.succeeded())) action.setExecutesAsync(True) reply = action.execute() if reply.failed(): return False return True Thanks, Yichao Yu
Re: Review Request 108308: 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. 16, 2013, 2:37 p.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
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? 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. - Yichao --- 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
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/ --- 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: 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.
--- 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: 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. I guess the question is not which languages are CJK but which languages have this problem. - Yichao --- 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, 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. 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 - Yichao --- 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
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/ --- 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.
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.) (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 - Yichao --- 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: Correctly trigger executed signal of KListWidget for both single click and double click.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107829/ --- (Updated Dec. 22, 2012, 7:21 p.m.) Review request for kdelibs. Changes --- Add related bug report. Description --- When clicking on a item in a KListWidget, the `executed` signals is not correctly emitted. If the system-wide setting is to use single click, only `executed( QListWidgetItem *item )` is emitted (once). If the setting is to use double click, `executed( QListWidgetItem *item )` will be emitted twice and `executed( QListWidgetItem *item, const QPoint pos )` is emitted once, which cause the edit note dialog in Kontact_KNotePlugin to popup a second time right after close in some cases. This patch fixes the problem. It is a little hacky but there isn't another easy way to get the position of the single click event except overriding the mouseReleaseEvent function. This addresses bug 201093. http://bugs.kde.org/show_bug.cgi?id=201093 Diffs - kdeui/itemviews/klistwidget.h 9309efc kdeui/itemviews/klistwidget.cpp 13497bf Diff: http://git.reviewboard.kde.org/r/107829/diff/ Testing --- Compiled kdelibs as well as program that has problem (kontact) and the signal is triggered correctly. Thanks, Yichao Yu
Re: Review Request: Correctly trigger executed signal of KListWidget for both single click and double click.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107829/ --- (Updated Dec. 22, 2012, 7:46 p.m.) Review request for kdelibs and Stephan Kulow. Description (updated) --- When clicking on a item in a KListWidget, the `executed` signals is not correctly emitted. If the system-wide setting is to use single click, only `executed( QListWidgetItem *item )` is emitted (once). If the setting is to use double click, `executed( QListWidgetItem *item )` will be emitted twice and `executed( QListWidgetItem *item, const QPoint pos )` is emitted once, which cause the edit note dialog in Kontact_KNotePlugin to popup a second time right after close in some cases. This patch fixes the problem. It is a little hacky but there isn't another easy way to get the position of the single click event except overriding the mouseReleaseEvent function AFAIK. According to git log, this bug was introduced by commit b4a7662da2ddd14c8f1a9c97dc65b25418a5c05b back in 2007. In the commit log, it says executed(item, pos); is no longer supported and should be removed CCMAIL: robertkni...@gmail.com But I cannot found this anywhere else, as least neither in the api document here[1] nor anywhere in the source code/headers (correct me if I am wrong). Is this signal really to-be-removed? (At least this patch makes it work again.) [1] http://api.kde.org/4.9-api/kdelibs-apidocs/kdeui/html/classKListWidget.html#a3d8fe2b4c4240e4073bd824e0599b24e This addresses bug 201093. http://bugs.kde.org/show_bug.cgi?id=201093 Diffs - kdeui/itemviews/klistwidget.h 9309efc kdeui/itemviews/klistwidget.cpp 13497bf Diff: http://git.reviewboard.kde.org/r/107829/diff/ Testing --- Compiled kdelibs as well as program that has problem (kontact) and the signal is triggered correctly. Thanks, Yichao Yu
Review Request: Correctly trigger executed signal of KListWidget for both single click and double click.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107829/ --- Review request for kdelibs. Description --- When clicking on a item in a KListWidget, the `executed` signals is not correctly emitted. If the system-wide setting is to use single click, only `executed( QListWidgetItem *item )` is emitted (once). If the setting is to use double click, `executed( QListWidgetItem *item )` will be emitted twice and `executed( QListWidgetItem *item, const QPoint pos )` is emitted once, which cause the edit note dialog in Kontact_KNotePlugin to popup a second time right after close in some cases. This patch fixes the problem. It is a little hacky but there isn't another easy way to get the position of the single click event except overriding the mouseReleaseEvent function. Diffs - kdeui/itemviews/klistwidget.h 9309efc kdeui/itemviews/klistwidget.cpp 13497bf Diff: http://git.reviewboard.kde.org/r/107829/diff/ Testing --- Compiled kdelibs as well as program that has problem (kontact) and the signal is triggered correctly. Thanks, Yichao Yu
Re: Review Request: Correctly trigger executed signal of KListWidget for both single click and double click.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107829/ --- (Updated Dec. 21, 2012, 4:19 p.m.) Review request for kdelibs. Changes --- NULL - 0 Description --- When clicking on a item in a KListWidget, the `executed` signals is not correctly emitted. If the system-wide setting is to use single click, only `executed( QListWidgetItem *item )` is emitted (once). If the setting is to use double click, `executed( QListWidgetItem *item )` will be emitted twice and `executed( QListWidgetItem *item, const QPoint pos )` is emitted once, which cause the edit note dialog in Kontact_KNotePlugin to popup a second time right after close in some cases. This patch fixes the problem. It is a little hacky but there isn't another easy way to get the position of the single click event except overriding the mouseReleaseEvent function. Diffs (updated) - kdeui/itemviews/klistwidget.h 9309efc kdeui/itemviews/klistwidget.cpp 13497bf Diff: http://git.reviewboard.kde.org/r/107829/diff/ Testing --- Compiled kdelibs as well as program that has problem (kontact) and the signal is triggered correctly. Thanks, Yichao Yu
