Question about HTTP KIO Slave
Reposting here in case someone has an idea: Hi all, I'm working on a fix for a bug we have in KGet, where some files are downloaded incomplete. I found that at least one of the reasons why this happens is because of the following use case. when KGet is set up as the download manager for Rekonq: 1. Rekonq requests a URL, compressed pages are allowed 2. Rekonq gets the MIME type and puts the slave on hold because it is not a supported type 3. KGet kicks in and a transfer plugin requests the same URL (no need to know what a transfer plugin is, just know it does not support compressed pages) 4. KLauncher decides the URL was already requested so it assigns the same slave 5. The slave reports the compessed transfer size, the transfer plugin gets the wrong size and the file is downloaded incomplete In that scenario, shouldn't the HTTP slave know that the new metadata is not compatible with the previous one and restart the request? I worked around this issue by reading response headers and killing/restarting the KIO::get when compressed encoding was detected, but that's of course ugly. Another trick I tried was to recognize the old metadata was not compatible with the new metadata at the slave level, and restart the request, but code was getting messy so I never finished that approach. Any ideas of what to do with this scenario? Can this be considered a bug in HTTP KIO Slave? David E. Narváez
Re: Re: KDEREVIEW: Mangonel
Hello List, On Tuesday 08 January 2013 23:12:01 Albert Astals Cid wrote: El Dimarts, 8 de gener de 2013, a les 21:08:11, Martin Sandsmark va escriure: Dear Sirs and Madams, Mangonel has just been moved to KDE Review. Mangonel is a simple and lightweight application launcher in the vein of Katapult from ye olde KDE 3 days, kind of reminiscent of the task oriented UI for krunner, but dropping the slow krunner architecture in favour of a simpler and more straightforward one that aims to give immediate feedback to the user. So it doesn't do everything krunner does, but should be useful for some people as either an alternative to or complement to krunner. It was originally developed by Bart Kroon, but he is currently too busy to work on it, so I got a go-ahead from him to clean it up for a proper release with licenses and stuff (which I think it needs because it's pretty useful and should be spread far and wide). Which is its intended destination extragear-something? Any reason not to use bugs.kde.org? It was not in KDE so it did not use bugs.kde.org. This would be appropriate when it is adopted obviously. The i18n is quite broken, a simple grep gives me ./Config.cpp:39:this-setWindowTitle(KApplication::instance()- applicationName() + QString( - Configuration)); ./Config.cpp:48:QLabel* hotkeyLabel = new QLabel(Shortcut to show Mangonel:, this); ./Mangonel.cpp:74:m_actionShow = new KAction(QString(Show Mangonel), this); ./Mangonel.cpp:92:QAction* actionConfig = new QAction(KIcon(configure), Configuration, this); ./Mangonel.cpp:480:m_descriptionLabel = new QGraphicsTextItem(( + application.type + ), this); Translations are indeed not a thing this program currently supports. There are not too many strings in there so this should not pose to much of a problem. Besides all those units in units.c seem untranslatable. Any reason for not using kunitconversion in there? I didn't know about this. This seems appropriate. Also the units.c copytight header looks a bit scary This was realy a quick hack add on that still needs a propper implementation. It doesn't even work properly as it is now. Cheers, Albert - Bart
Re: Review Request: Fix KWindowSystem::compositingChanged signal
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107983/#review25012 --- after I apply the patch, I notice that in some random unknnow case KWindowSystem will emit the signal, while KSelectionOwner is not. (And I think KSelectionOwner is correct since I didn't do anything to kwin), and I notice some strange plasma theme change caused by this. Trying to find you why.. - Xuetian Weng On Jan. 5, 2013, 5:08 p.m., Thomas Lübking wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107983/ --- (Updated Jan. 5, 2013, 5:08 p.m.) Review request for kdelibs, kwin, Plasma, Aaron J. Seigo, Marco Martin, Martin Gräßlin, and Fredrik Höglund. Description --- It works fine here (tested so far KWindowSystem signal, KSelectionWatcher only with kwin) with kwin (shift+alt+f12), xcompmgr, compiz metacity -c and e17. Didn't try xfce nor mutter. Technically: I do not at all understand why KWindowSystem is *not* watching the root window - KSelectionOwner for one is sending events to the root and this also seems the case for all other WMs (at least everything now starts to cause the signal to be emitted) The KSelectionWatcher failure seems to be kwin specific (wrote me a cleaner testcase), there'll be some X11 event processing on top that eats away the client messages. So this one can be scratched from the patch, the KWindowSystem issue remains. This addresses bug 179042. http://bugs.kde.org/show_bug.cgi?id=179042 Diffs - kdeui/windowmanagement/kwindowsystem_x11.cpp f9b3cc1 Diff: http://git.reviewboard.kde.org/r/107983/diff/ Testing --- see summary Thanks, Thomas Lübking
Re: Review Request: Fix KWindowSystem::compositingChanged signal
On Jan. 8, 2013, 11:15 p.m., Xuetian Weng wrote:
after I apply the patch, I notice that in some random unknnow case
KWindowSystem will emit the signal, while KSelectionOwner is not. (And I
think KSelectionOwner is correct since I didn't do anything to kwin), and I
notice some strange plasma theme change caused by this.
Trying to find you why..
Thomas Lübking wrote:
The XFixesSelectionNotify event fires one every selection owner event,
usually it'll be the cnp system (when you eg. mark text)
The problem should ideally be fixed in Qt instead of working around it in
KDE.
Untested patch:
diff --git a/src/gui/kernel/qapplication_x11.cpp
b/src/gui/kernel/qapplication_x11.cpp
index 3631f1d..644f0ad 100644
--- a/src/gui/kernel/qapplication_x11.cpp
+++ b/src/gui/kernel/qapplication_x11.cpp
@@ -536,6 +536,7 @@ struct qt_xfixes_selection_event_data
{
// which selection to filter out.
Atom selection;
+Window window;
};
#if defined(Q_C_CALLBACKS)
@@ -548,7 +549,7 @@ static Bool qt_xfixes_scanner(Display*, XEvent
*event, XPointer arg)
reinterpret_castqt_xfixes_selection_event_data*(arg);
if (event-type == X11-xfixes_eventbase + XFixesSelectionNotify) {
XFixesSelectionNotifyEvent *xfixes_event =
reinterpret_castXFixesSelectionNotifyEvent*(event);
-if (xfixes_event-selection == data-selection)
+if (xfixes_event-selection == data-selection
xfixes_event-window == data-window)
return true;
}
return false;
@@ -3462,6 +3463,7 @@ int QApplication::x11ProcessEvent(XEvent* event)
// we don't want to handle old SelectionNotify events.
qt_xfixes_selection_event_data xfixes_event;
xfixes_event.selection = req-selection;
+xfixes_event.window = req-window;
for (XEvent ev;;) {
if (!XCheckIfEvent(X11-display, ev, qt_xfixes_scanner,
(XPointer)xfixes_event))
break;
but I don't see in near future there will be Qt 4.8.5 nor distro can pick up
this patch..
actually this can cause serious problem, clipboard is so widely used.
I guess if would be better to workaround it for now, I already notice some
strange theme change, since clipboard is being initialized by KApplication by
default..
- Xuetian
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/107983/#review25012
---
On Jan. 5, 2013, 5:08 p.m., Thomas Lübking wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/107983/
---
(Updated Jan. 5, 2013, 5:08 p.m.)
Review request for kdelibs, kwin, Plasma, Aaron J. Seigo, Marco Martin,
Martin Gräßlin, and Fredrik Höglund.
Description
---
It works fine here (tested so far KWindowSystem signal, KSelectionWatcher
only with kwin) with kwin (shift+alt+f12), xcompmgr, compiz metacity -c
and e17.
Didn't try xfce nor mutter.
Technically:
I do not at all understand why KWindowSystem is *not* watching the root
window - KSelectionOwner for one is sending events to the root and this also
seems the case for all other WMs (at least everything now starts to cause the
signal to be emitted)
The KSelectionWatcher failure seems to be kwin specific (wrote me a cleaner
testcase), there'll be some X11 event processing on top that eats away the
client messages.
So this one can be scratched from the patch, the KWindowSystem issue remains.
This addresses bug 179042.
http://bugs.kde.org/show_bug.cgi?id=179042
Diffs
-
kdeui/windowmanagement/kwindowsystem_x11.cpp f9b3cc1
Diff: http://git.reviewboard.kde.org/r/107983/diff/
Testing
---
see summary
Thanks,
Thomas Lübking
Re: Review Request: Fix KWindowSystem::compositingChanged signal
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107983/ --- (Updated Jan. 9, 2013, 12:52 a.m.) Review request for kdelibs, kwin, Plasma, Aaron J. Seigo, Marco Martin, Martin Gräßlin, and Fredrik Höglund. Changes --- Apparently this patch is actually used, so turn it into a correct workaround. It's btw. called review board, folks, not secret bugfree version of kde board ;-) Description --- It works fine here (tested so far KWindowSystem signal, KSelectionWatcher only with kwin) with kwin (shift+alt+f12), xcompmgr, compiz metacity -c and e17. Didn't try xfce nor mutter. Technically: I do not at all understand why KWindowSystem is *not* watching the root window - KSelectionOwner for one is sending events to the root and this also seems the case for all other WMs (at least everything now starts to cause the signal to be emitted) The KSelectionWatcher failure seems to be kwin specific (wrote me a cleaner testcase), there'll be some X11 event processing on top that eats away the client messages. So this one can be scratched from the patch, the KWindowSystem issue remains. This addresses bug 179042. http://bugs.kde.org/show_bug.cgi?id=179042 Diffs (updated) - kdeui/windowmanagement/kwindowsystem_x11.cpp f9b3cc1 Diff: http://git.reviewboard.kde.org/r/107983/diff/ Testing --- see summary Thanks, Thomas Lübking
Re: KDEREVIEW: Mangonel
On 2013年01月09日 08:09, Martin Sandsmark wrote: Any reason not to use bugs.kde.org? Fixed. Hi, I see you made the change : -aboutData-setBugAddress(QByteArray(bugs.mango...@tarmack.eu)); +aboutData-setBugAddress(QByteArray(https://bugs.kde.org/;)); Hmm, that is not going to work. Dr.Konqi expects sub...@bugs.kde.org as the bug address of applications using bugs.kde.org. If you plan to use bugs.kde.org as the tracker, then you don't need to call setBugAddress() at all. The default value just works. And don't forget to ask sysadmins to create a mangonel product on bugs.kde.org :) Regards Jekyll
Re: Review Request: Fix KWindowSystem::compositingChanged signal
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107983/#review25025 --- kdeui/windowmanagement/kwindowsystem_x11.cpp http://git.reviewboard.kde.org/r/107983/#comment19169 I thought you should check event-selection == net_wm_cm here, no? kdeui/windowmanagement/kwindowsystem_x11.cpp http://git.reviewboard.kde.org/r/107983/#comment19170 I don't quite get why this part is required. Since KWindowSystem already calls SelectSelectionInput in constructor, just use the above one and do window == winId and event-selection == net_wm_cm is enough. - Xuetian Weng On Jan. 9, 2013, 12:52 a.m., Thomas Lübking wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107983/ --- (Updated Jan. 9, 2013, 12:52 a.m.) Review request for kdelibs, kwin, Plasma, Aaron J. Seigo, Marco Martin, Martin Gräßlin, and Fredrik Höglund. Description --- It works fine here (tested so far KWindowSystem signal, KSelectionWatcher only with kwin) with kwin (shift+alt+f12), xcompmgr, compiz metacity -c and e17. Didn't try xfce nor mutter. Technically: I do not at all understand why KWindowSystem is *not* watching the root window - KSelectionOwner for one is sending events to the root and this also seems the case for all other WMs (at least everything now starts to cause the signal to be emitted) The KSelectionWatcher failure seems to be kwin specific (wrote me a cleaner testcase), there'll be some X11 event processing on top that eats away the client messages. So this one can be scratched from the patch, the KWindowSystem issue remains. This addresses bug 179042. http://bugs.kde.org/show_bug.cgi?id=179042 Diffs - kdeui/windowmanagement/kwindowsystem_x11.cpp f9b3cc1 Diff: http://git.reviewboard.kde.org/r/107983/diff/ Testing --- see summary Thanks, Thomas Lübking
Re: Review Request: Fix KWindowSystem::compositingChanged signal
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107983/ --- (Updated Jan. 9, 2013, 2 a.m.) Review request for kdelibs, kwin, Plasma, Aaron J. Seigo, Marco Martin, Martin Gräßlin, and Fredrik Höglund. Changes --- only watch net_wm_cm selections, the workaround and regular code are not migrated by intent Description --- It works fine here (tested so far KWindowSystem signal, KSelectionWatcher only with kwin) with kwin (shift+alt+f12), xcompmgr, compiz metacity -c and e17. Didn't try xfce nor mutter. Technically: I do not at all understand why KWindowSystem is *not* watching the root window - KSelectionOwner for one is sending events to the root and this also seems the case for all other WMs (at least everything now starts to cause the signal to be emitted) The KSelectionWatcher failure seems to be kwin specific (wrote me a cleaner testcase), there'll be some X11 event processing on top that eats away the client messages. So this one can be scratched from the patch, the KWindowSystem issue remains. This addresses bug 179042. http://bugs.kde.org/show_bug.cgi?id=179042 Diffs (updated) - kdeui/windowmanagement/kwindowsystem_x11.cpp f9b3cc1 Diff: http://git.reviewboard.kde.org/r/107983/diff/ Testing --- see summary Thanks, Thomas Lübking
Re: Review Request: Fix bug 234407 - systemsettings iconview wordwrapping cause narrow icon in zhcn locale
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108285/#review25038 --- Code itself looks okay, see issue below though. systemsettings/icons/IconMode.cpp http://git.reviewboard.kde.org/r/108285/#comment19173 Not sure I like the idea of a hardcoded list of languages... is there a better way of determining if a language is CJK? - Ben Cooksley 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: set brightness to zero in profile doesn't work
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108230/#review25051 --- This review has been submitted with commit 55b759ab8b9f2d45e486fc84495682f00d9326d0 by Weng Xuetian to branch KDE/4.10. - Commit Hook On Jan. 6, 2013, 6 p.m., Xuetian Weng wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108230/ --- (Updated Jan. 6, 2013, 6 p.m.) Review request for kde-workspace and Kai Uwe Broulik. Description --- fix is trivial, since -1 is the invalid value in this part of code, not zero. Uncheck the brightness will set m_defaultValue to -1 so no function is lost. This addresses bug 306925. http://bugs.kde.org/show_bug.cgi?id=306925 Diffs - powerdevil/daemon/actions/bundled/brightnesscontrol.cpp 5088e4e Diff: http://git.reviewboard.kde.org/r/108230/diff/ Testing --- now zero in profile works. 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? CJK is actually naming the languages which use CJK, so the list is hardcoded by definition. - Christoph --- 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: KDEREVIEW: Mangonel
On Wednesday 09 January 2013 08:56:25 Jekyll Wu wrote: If you plan to use bugs.kde.org as the tracker, then you don't need to call setBugAddress() at all. The default value just works. Fixed. And don't forget to ask sysadmins to create a mangonel product on bugs.kde.org :) Done. Thanks for the review! :D -- Martin Sandsmark KDE
Re: Question about HTTP KIO Slave
El Dimarts, 8 de gener de 2013, a les 17:17:33, David Narvaez va escriure: Reposting here in case someone has an idea: Allan answered you in kde-devel weeks ago. Cheers, Albert
Re: Review Request: Fix bug 234407 - systemsettings iconview wordwrapping cause narrow icon in zhcn locale
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108285/#review25091 --- 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 - Albert Astals Cid 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: Re: KDEREVIEW: Mangonel
On Wednesday 09 January 2013 18:35:49 Martin Sandsmark wrote: On Tuesday 08 January 2013 19:30:45 Allen Winter wrote: No docbook manual I guess I'll to contact the doc team for this? Do you want apidox generated? if so you also need a Mainpage.dox No need for that (yet, at least, we might want to make it plugin-based in the future). Bart's email address is missing from the Copyright statements in many files. On purpose, I didn't want to spread his email address unencoded all over the internet. There is one mail already on the list, so it is already spread. signature.asc Description: This is a digitally signed message part.
Re: KDEREVIEW: Mangonel
El Dimecres, 9 de gener de 2013, a les 01:09:28, Martin Sandsmark va escriure: Hi, thanks for the review! On Tuesday 08 January 2013 23:12:01 Albert Astals Cid wrote: Which is its intended destination extragear-something? Yes, sorry, I forgot to mention, it is destined for extragear/base. Any reason not to use bugs.kde.org? Fixed. The i18n is quite broken, a simple grep gives me ./Config.cpp:39:this-setWindowTitle(KApplication::instance()- applicationName() + QString( - Configuration)); ./Config.cpp:48:QLabel* hotkeyLabel = new QLabel(Shortcut to show Mangonel:, this); ./Mangonel.cpp:74:m_actionShow = new KAction(QString(Show Mangonel), this); ./Mangonel.cpp:92:QAction* actionConfig = new QAction(KIcon(configure), Configuration, this); Fixed. ./Mangonel.cpp:480:m_descriptionLabel = new QGraphicsTextItem(( + application.type + ), this); application.type should already be translated, is it problematic that I wrap it in parentheses? You should probably make it look like i18nc(some context of what stuff is, (%1), application.type) Just in case someone needs to do something different with the parenthesis. Besides all those units in units.c seem untranslatable. Any reason for not using kunitconversion in there? Ported it to use KUnitConversion now. Also the units.c copytight header looks a bit scary Since they're not necessary anymore I just deleted units.c/units.h. (I hope I don't need to eradicate them from the git history?) No idea if we really need to be such purists Cheers, Albert And lastly, I also forgot to thank Bart for this great app. :-)
Re: KDEREVIEW: Mangonel
On Wednesday 09 January 2013 19:49:36 Albert Astals Cid wrote: You should probably make it look like i18nc(some context of what stuff is, (%1), application.type) Just in case someone needs to do something different with the parenthesis. Okay, fixed. Thanks :-) -- Martin Sandsmark KDE
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 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 ;-) - Thomas --- 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
Re: KDEREVIEW: Mangonel
On Wednesday 09 January 2013 19:45:27 Martin Gräßlin wrote: There is one mail already on the list, so it is already spread. Okay, so the proverbial cat's out of the bag I guess, so I just added his email to the license headers. -- Martin Sandsmark KDE
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/#review25103 --- 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.) - Thomas Lübking 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: 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
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 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. - Xuetian --- 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: KDEREVIEW: Mangonel
написане Wed, 09 Jan 2013 19:35:49 +0200, Martin Sandsmark martin.sandsm...@kde.org: On Tuesday 08 January 2013 19:30:45 Allen Winter wrote: No docbook manual I guess I'll to contact the doc team for this? If there is no Help button and no desktop file, there is not much sense in making docbooks. I propose just add an item to UserBase launchers list [1] and some tiny page based on README.md. Just my two cents. Best regards, Yuri [1] http://userbase.kde.org/Plasma_application_launchers
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: 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 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 - Thomas --- 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: KDEREVIEW: Mangonel
On Wednesday 09 January 2013 09:07:13 PM Yuri Chornoivan wrote: написане Wed, 09 Jan 2013 19:35:49 +0200, Martin Sandsmark martin.sandsm...@kde.org: On Tuesday 08 January 2013 19:30:45 Allen Winter wrote: No docbook manual I guess I'll to contact the doc team for this? If there is no Help button and no desktop file, there is not much sense in making docbooks. I propose just add an item to UserBase launchers list [1] and some tiny page based on README.md. Good idea. [1] http://userbase.kde.org/Plasma_application_launchers
Re: Review Request: fix isMinimized in KWindowInfo
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108314/#review25109 --- There should be no problem with ths one since WMs are encouraged (should, *sigh*) to set _NET_WM_HIDDEN for minimized windows and the major ones do. If _NET_WM_STATE is absent it's considered 0, thus does not match HIDDEN, thus passes the check over to the ICCCM state. The weird condition is when a window has HIDDEN but not inconified, just that this is what the patch uses a fix. No apparent concerns from my side. - Thomas Lübking On Jan. 9, 2013, 11:25 p.m., Xuetian Weng wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108314/ --- (Updated Jan. 9, 2013, 11:25 p.m.) 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: 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/#review25110 --- Copied over from https://git.reviewboard.kde.org/r/108308/ There should be no problem with ths one since WMs are encouraged (should, *sigh*) to set _NET_WM_HIDDEN for minimized windows and the major ones do. If _NET_WM_STATE is absent it's considered 0, thus does not match HIDDEN, thus passes the check over to the ICCCM state. The weird condition is when a window has HIDDEN but not inconified, just that this is what the patch uses a fix. No apparent concerns from my side. - Thomas Lübking On Jan. 10, 2013, 12:12 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, 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 --- Compiled, pager and icontasks shows minimized windows correctly. Also tested on openbox (+plasma's pager) by Xuetian Weng. 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/#review25123 --- 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. - Martin Gräßlin On Jan. 10, 2013, 12:12 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, 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 --- Compiled, pager and icontasks shows minimized windows correctly. Also tested on openbox (+plasma's pager) by Xuetian Weng. 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, 6:50 a.m.) Review request for kdelibs, kwin, Plasma, Aaron J. Seigo, and Martin Gräßlin. Changes --- added plasma to CC group - I think they should be in to comment on it. 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
