Review Request: Use the focus proxy, if present, as parent of KonqPopupMenu
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104890/ --- Review request for KDE Base Apps and David Faure. Description --- The following patch changes the parent widget used when creating a context menu in Konqueror. This is done to prevent unnecessary redraw that seems to occur when clicking on a kwebkitpart component that is currently displaying a flash movie. See bug#298744 for further details. What makes the problem even more confounding is the fact that if the page playing the flash movie is reloaded, either before or after clicking the RMB, then clicking the RMB button to display the context menu afterwards works just fine. It is just the first click that causes the bug. This addresses bug 298744. http://bugs.kde.org/show_bug.cgi?id=298744 Diffs - konqueror/src/konqmainwindow.cpp ea1678b Diff: http://git.reviewboard.kde.org/r/104890/diff/ Testing --- Made sure the bug reported in bug #298744 is gone after testing the patch. Thanks, Dawit Alemayehu
Re: Review Request: Use the focus proxy, if present, as parent of KonqPopupMenu
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104890/#review13602 --- This makes no sense to me. Popping up a context menu has nothing to do with keyboard focus, and the actual cause for the bug hasn't been identified, apparently. So this is a blind workaround, which I cannot approve. - David Faure On May 9, 2012, 7:13 a.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104890/ --- (Updated May 9, 2012, 7:13 a.m.) Review request for KDE Base Apps and David Faure. Description --- The following patch changes the parent widget used when creating a context menu in Konqueror. This is done to prevent unnecessary redraw that seems to occur when clicking on a kwebkitpart component that is currently displaying a flash movie. See bug#298744 for further details. What makes the problem even more confounding is the fact that if the page playing the flash movie is reloaded, either before or after clicking the RMB, then clicking the RMB button to display the context menu afterwards works just fine. It is just the first click that causes the bug. This addresses bug 298744. http://bugs.kde.org/show_bug.cgi?id=298744 Diffs - konqueror/src/konqmainwindow.cpp ea1678b Diff: http://git.reviewboard.kde.org/r/104890/diff/ Testing --- Made sure the bug reported in bug #298744 is gone after testing the patch. Thanks, Dawit Alemayehu
Re: Review Request: dataprotocol: make charset recoding work
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104874/#review13603 --- kio/kio/dataprotocol.cpp http://git.reviewboard.kde.org/r/104874/#comment10793 Not sure the comment is still correct. decoded? It's not, anymore, right? It's just extracted? Or is it even the full initial URL? kio/kio/dataprotocol.cpp http://git.reviewboard.kde.org/r/104874/#comment10792 Why .toUtf8()? Are we sure that this is what the receiver of the data will use, for decoding? (I mean in the real case of an application getting the data, not in the unittest) kio/kio/dataprotocol.cpp http://git.reviewboard.kde.org/r/104874/#comment10791 This comment isn't applicable anymore, it was the justification for toLocal8Bit(). - David Faure On May 6, 2012, 6:14 p.m., Rolf Eike Beer wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104874/ --- (Updated May 6, 2012, 6:14 p.m.) Review request for kdelibs. Description --- This reworks the code that works with different character sets to actually do the right thing (tm). Diffs - kio/kio/dataprotocol.cpp e614476 kio/tests/dataprotocoltest.cpp c8df132 Diff: http://git.reviewboard.kde.org/r/104874/diff/ Testing --- -build whole kdelibs -added more testcases from http://greenbytes.de/tech/tc/datauri -all dataprotocol tests pass Thanks, Rolf Eike Beer
Re: Review Request: dataprotocol: simplify helper code
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104860/#review13604
---
OK, except for const QChar .
kio/kio/dataprotocol.cpp
http://git.reviewboard.kde.org/r/104860/#comment10794
Not sure a const ref is a good idea, for a QChar (which is basically a
ushort).
E.g. QString::at() and [] return a QChar, not a const ref.
kio/kio/dataprotocol.cpp
http://git.reviewboard.kde.org/r/104860/#comment10795
same here
- David Faure
On May 6, 2012, 6:10 p.m., Rolf Eike Beer wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104860/
---
(Updated May 6, 2012, 6:10 p.m.)
Review request for kdelibs.
Description
---
-add some const and static
-remove function parameters that always have the same values, use local
statics
in the function to hold these
-QChar(QLatin1Char('\0')) = QChar()
-QChar == QLatin1Char('\0') = QChar::isNull()
Diffs
-
kio/kio/dataprotocol.cpp e614476
Diff: http://git.reviewboard.kde.org/r/104860/diff/
Testing
---
-build whole kdelibs
-dataprotocol testcases still pass
Thanks,
Rolf Eike Beer
Re: Re: Package Dependcies List on Techbase
Please excuse the Top-Post, my suggestion is VERY short: The terminology Optional Dependency sounds like a good term for these situations. (At least to me, a native 'en-us' person.) On 05/08/2012 06:18 AM, David Jarvie wrote: On Tue, May 8, 2012 1:07 pm, Allen Winter wrote: On Tuesday 08 May 2012 6:55:01 AM David Jarvie wrote: On Mon, May 7, 2012 4:36 pm, Allen Winter wrote: Howdy, I started putting the list of package dependences (arranged by module) onto Techbase. http://techbase.kde.org/Getting_Started/Build/Requirements The tables on the subpages there are generated by a perl program I wrote. That program reads the CMakeLists.txt files inside a module and generates wiki content I then copy+paste into Techbase. Please review for accuracy. 2) Is QtDeclarative actually REQUIRED for kdepim? Isn't it only required in order to build mobile apps? If so, it should be marked as Optional. Are there any other dependencies which are similarly marked as Required, when in fact they are optional? Well, I'm not planning to write a CMakeLists.txt parser. So I'm not planning to handle CMake conditionals. But I can add hacks as needed. In the case of QtDeclarative, the comment says that it is needed for Mobile. Making sure we have useful comments and descriptions can certainly help too. Yes, the comment says that it is for mobile, but Required is a strong term, and I don't think the comment in its current form makes it clear enough that Required might not actually mean what it says. In this particular example, QtDeclarative will not be needed for someone building for the desktop. This will be the default build option for many people, so I think it needs to be stated more explicitly that Required may actually mean Optional. I can appreciate that you may not have time to write a parser for cmake conditionals. But if conditional dependencies are going to be listed as Required, I think there should be a clear statement at the top of the page that Required doesn't necessarily mean what it says, and may mean optional, depending on what conditional settings are used.
Re: Review Request: dataprotocol: make charset recoding work
On May 9, 2012, 12:04 p.m., David Faure wrote: kio/kio/dataprotocol.cpp, line 71 http://git.reviewboard.kde.org/r/104874/diff/1/?file=63068#file63068line71 Not sure the comment is still correct. decoded? It's not, anymore, right? It's just extracted? Or is it even the full initial URL? Yes, it is still correct. This is after the percent encoding has been decoded. On May 9, 2012, 12:04 p.m., David Faure wrote: kio/kio/dataprotocol.cpp, line 277 http://git.reviewboard.kde.org/r/104874/diff/1/?file=63068#file63068line277 Why .toUtf8()? Are we sure that this is what the receiver of the data will use, for decoding? (I mean in the real case of an application getting the data, not in the unittest) I have used the new testcases on the old code and compared with the online tests. The only cases where old code was right and the string afterwards was displayed correctly was when it returned UTF8 strings. So returning UTF8 is the right thing here IMHO. On May 9, 2012, 12:04 p.m., David Faure wrote: kio/kio/dataprotocol.cpp, line 279 http://git.reviewboard.kde.org/r/104874/diff/1/?file=63068#file63068line279 This comment isn't applicable anymore, it was the justification for toLocal8Bit(). Correct, will delete that. - Rolf Eike --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104874/#review13603 --- On May 6, 2012, 6:14 p.m., Rolf Eike Beer wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104874/ --- (Updated May 6, 2012, 6:14 p.m.) Review request for kdelibs. Description --- This reworks the code that works with different character sets to actually do the right thing (tm). Diffs - kio/kio/dataprotocol.cpp e614476 kio/tests/dataprotocoltest.cpp c8df132 Diff: http://git.reviewboard.kde.org/r/104874/diff/ Testing --- -build whole kdelibs -added more testcases from http://greenbytes.de/tech/tc/datauri -all dataprotocol tests pass Thanks, Rolf Eike Beer
KDE 4.9 feature plan
Hi all, It would be great if you could all take a few minutes to update the feature plan for 4.9 (preferably right now or you'll forget again!) http://techbase.kde.org/Schedules/KDE4/4.9_Feature_Plan as the Quality Team would need to identify new features, new applets, new apps,.. in order to give them some priority testing! Can you also forward this to any relevant subgroup you know, thanks a lot in advance. Best regards, Anne-Marie
Re: Review Request: Use the focus proxy, if present, as parent of KonqPopupMenu
On May 9, 2012, 7:37 a.m., David Faure wrote: This makes no sense to me. Popping up a context menu has nothing to do with keyboard focus, and the actual cause for the bug hasn't been identified, apparently. So this is a blind workaround, which I cannot approve. Well this is most definitely not about a keyboard focus issue. Rather it is an attempt to find a way to make QWebView the parent of the popup menu instead of the KPart's widget. The KPart's widget in most cases (at least in khtml and kwebkitpart), uses the actual content viewing widget its focus proxy. That is why this patch attempts to use the focus proxy widget when available. However, you are correct. This is a workaround for a yet to be identified issue with the entire QWebView flickering and losing the flash plugin display when you attempt to popup a context menu using the KPart's widget as the parent of the popup menu. That problem does not occur if the QWebView is used as the parent widget of the context menu. No clue why this happens though. What is even more bizarre is if you simply reload the page and right click again, you will not see the flickering again. It only happens the first time the context menu is shown a page embedding a flash plugin. - Dawit --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104890/#review13602 --- On May 9, 2012, 7:13 a.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104890/ --- (Updated May 9, 2012, 7:13 a.m.) Review request for KDE Base Apps and David Faure. Description --- The following patch changes the parent widget used when creating a context menu in Konqueror. This is done to prevent unnecessary redraw that seems to occur when clicking on a kwebkitpart component that is currently displaying a flash movie. See bug#298744 for further details. What makes the problem even more confounding is the fact that if the page playing the flash movie is reloaded, either before or after clicking the RMB, then clicking the RMB button to display the context menu afterwards works just fine. It is just the first click that causes the bug. This addresses bug 298744. http://bugs.kde.org/show_bug.cgi?id=298744 Diffs - konqueror/src/konqmainwindow.cpp ea1678b Diff: http://git.reviewboard.kde.org/r/104890/diff/ Testing --- Made sure the bug reported in bug #298744 is gone after testing the patch. Thanks, Dawit Alemayehu
Re: Review Request: dataprotocol: simplify helper code
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104860/#review13611
---
This review has been submitted with commit
279694530bfd6df3b8c5a8b31d1350bd2720a81f by Rolf Eike Beer to branch KDE/4.8.
- Commit Hook
On May 6, 2012, 6:10 p.m., Rolf Eike Beer wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104860/
---
(Updated May 6, 2012, 6:10 p.m.)
Review request for kdelibs.
Description
---
-add some const and static
-remove function parameters that always have the same values, use local
statics
in the function to hold these
-QChar(QLatin1Char('\0')) = QChar()
-QChar == QLatin1Char('\0') = QChar::isNull()
Diffs
-
kio/kio/dataprotocol.cpp e614476
Diff: http://git.reviewboard.kde.org/r/104860/diff/
Testing
---
-build whole kdelibs
-dataprotocol testcases still pass
Thanks,
Rolf Eike Beer
Review Request: Plasma Components: TextField and TextArea: Show placeholders even if item has the focus
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104895/ --- Review request for KDE Runtime. Description --- Rationale: This allows the application to pre-focus a text field with a placeholder text for the user. In the version before this would have hidden the placeholder text and it may not have been obvious for user what he was expected to enter in the text field. Diffs - plasma/declarativeimports/plasmacomponents/qml/TextArea.qml 2d9e89f plasma/declarativeimports/plasmacomponents/qml/TextField.qml 4ed15d9 Diff: http://git.reviewboard.kde.org/r/104895/diff/ Testing --- Used it in ktouch/next, works fine. Screenshots --- Form in KTouch http://git.reviewboard.kde.org/r/104895/s/562/ Thanks, Sebastian Gottfried
Re: Review Request: Plasma Components buttons: first focus, than emit clicked() signal
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104893/#review13616 --- I am unsure whether Plasma Components related patches are relevant to k-c-d. I would rather include the plasma devel team for reviews. Other Aaron, Marco or other plasma developers might miss this. :-) - Laszlo Papp On May 9, 2012, 4:41 p.m., Sebastian Gottfried wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104893/ --- (Updated May 9, 2012, 4:41 p.m.) Review request for KDE Runtime. Description --- Otherwise a client wanting to give another QML component the focus in reaction to a clicked button has no chance doing so because the button will steal the focus again right after the event hander has finished executing. Diffs - plasma/declarativeimports/plasmacomponents/qml/Button.qml 6aab1b2 plasma/declarativeimports/plasmacomponents/qml/ToolButton.qml 00ffa4c Diff: http://git.reviewboard.kde.org/r/104893/diff/ Testing --- Tested the behaviour in ktouch/next, works fine. Thanks, Sebastian Gottfried
Re: Review Request: Plasma Components: TextField and TextArea: Show placeholders even if item has the focus
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104895/#review13617 --- Ehh optional perhaps? I've seen forms behave like this before and back then when i first saw it i tried to delete the text.. Which obviously didn't happen. The text just disappears as soon as i start typing. I kinda dislike that behavior. I mean, there is a big fat blue focus border that has the purpose of telling the user that the one with the blue border (style dependent) is the one that currently has focus. The blinking cursor is yet another indication that the field has focus. I think that is enough. Perhaps optional, but please not by default. This is just my opinion and i don't maintain plasma components (nor anything in KDE for that matter). You'd have to wait for a reply from some of the plasma component maintainers to get a final word on this. - Mark Gaiser On May 9, 2012, 4:53 p.m., Sebastian Gottfried wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104895/ --- (Updated May 9, 2012, 4:53 p.m.) Review request for KDE Runtime. Description --- Rationale: This allows the application to pre-focus a text field with a placeholder text for the user. In the version before this would have hidden the placeholder text and it may not have been obvious for user what he was expected to enter in the text field. Diffs - plasma/declarativeimports/plasmacomponents/qml/TextArea.qml 2d9e89f plasma/declarativeimports/plasmacomponents/qml/TextField.qml 4ed15d9 Diff: http://git.reviewboard.kde.org/r/104895/diff/ Testing --- Used it in ktouch/next, works fine. Screenshots --- Form in KTouch http://git.reviewboard.kde.org/r/104895/s/562/ Thanks, Sebastian Gottfried
Re: Review Request: dataprotocol: make charset recoding work
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104874/#review13620 --- This review has been submitted with commit 7fce249425be95ce3b4e47e5c2393e64d793c13f by Rolf Eike Beer to branch KDE/4.8. - Commit Hook On May 6, 2012, 6:14 p.m., Rolf Eike Beer wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104874/ --- (Updated May 6, 2012, 6:14 p.m.) Review request for kdelibs. Description --- This reworks the code that works with different character sets to actually do the right thing (tm). Diffs - kio/kio/dataprotocol.cpp e614476 kio/tests/dataprotocoltest.cpp c8df132 Diff: http://git.reviewboard.kde.org/r/104874/diff/ Testing --- -build whole kdelibs -added more testcases from http://greenbytes.de/tech/tc/datauri -all dataprotocol tests pass Thanks, Rolf Eike Beer
Review Request: Move the Preferred Web shorcut selection checkbox into its own column
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104900/ --- Review request for KDE Runtime. Description --- The following patch moves the Preferred/Favorite web shortcut selection checkbox into its own column to avoid confusion. The new column is marked as Preferred and also shows a tool tip message about is functionality. See the screenshot below. This addresses bugs 168223 and 218164. http://bugs.kde.org/show_bug.cgi?id=168223 http://bugs.kde.org/show_bug.cgi?id=218164 Diffs - kurifilter-plugins/ikws/ikwsopts.cpp f1cc481 kurifilter-plugins/ikws/ikwsopts_p.h 9cfc12c kurifilter-plugins/ikws/ikwsopts_ui.ui 440c201 Diff: http://git.reviewboard.kde.org/r/104900/diff/ Testing --- Screenshots --- Preferred selection column http://git.reviewboard.kde.org/r/104900/s/563/ Thanks, Dawit Alemayehu
Re: Review Request: Fix KShortcut to really allow the usage of multiple shortcuts
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104801/#review13624 --- Hate to break the news to you after all that work. But the exact same thing you have achieved with this patch could be achieved by using a QAction for the missing Shortcut. Or a KAction for that matter as long as it is not part of the KActionCollection. There is only one reason to use KAction. The Shortcuts Editor. And KXMLwhatever. And global shortcuts. So make that there are only three reasons to use KAction. None of them know how to handle a KAction with three Shortcuts set. So your patch has not (yet?) achieved anything you could not gain by just using a hidden unconfigurable second Q(K)Action. So i would say it does not make sense to apply it in its current form. Unless you are willing to go all the way which you only should do after finding out what the frameworks branchs does to kaction. So you effort is not thrown away in the near / middle future. Mike - Michael Jansen On May 9, 2012, 6:21 p.m., Mark Gaiser wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104801/ --- (Updated May 9, 2012, 6:21 p.m.) Review request for kdelibs. Description --- So i was trying to fix this bug: https://bugs.kde.org/show_bug.cgi?id=181531 That only asked for one more shortcut. That issue seems to be a little more complicated than it looks. Till this point KActions could only have a Primary and a Alternate shortcut. 2 in total which is - in some situations - not enough. I fixed this by roughly restructuring nearly all of the KShortcut cpp file. The only thing i'm not quite sure about is how KShortcut KAction::shortcut(ShortcutTypes type) const looks right now.. If anyone has some clarification on that one..? Diffs - kdeui/actions/kaction.h d877554 kdeui/actions/kaction.cpp 309cf82 kdeui/shortcuts/kshortcut.h c720830 kdeui/shortcuts/kshortcut.cpp e307ab0 Diff: http://git.reviewboard.kde.org/r/104801/diff/ Testing --- I tested this by adding the missing bindings for Dolphin's back/forward and it seems to be working just fine. I can use all available shortcuts. Thanks, Mark Gaiser
Re: Review Request: Plasma Components buttons: first focus, than emit clicked() signal
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104893/#review13626 --- Ship it! Good catch, please commit to KDE/4.8 and master or let me know so I'll do it for you. - Sebastian Kügler On May 9, 2012, 4:41 p.m., Sebastian Gottfried wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104893/ --- (Updated May 9, 2012, 4:41 p.m.) Review request for KDE Runtime. Description --- Otherwise a client wanting to give another QML component the focus in reaction to a clicked button has no chance doing so because the button will steal the focus again right after the event hander has finished executing. Diffs - plasma/declarativeimports/plasmacomponents/qml/Button.qml 6aab1b2 plasma/declarativeimports/plasmacomponents/qml/ToolButton.qml 00ffa4c Diff: http://git.reviewboard.kde.org/r/104893/diff/ Testing --- Tested the behaviour in ktouch/next, works fine. Thanks, Sebastian Gottfried
Re: Review Request: Move the Preferred Web shorcut selection checkbox into its own column
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104900/#review13627 --- Ship it! Ship It! - Andrea Diamantini On May 9, 2012, 8:35 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104900/ --- (Updated May 9, 2012, 8:35 p.m.) Review request for KDE Runtime. Description --- The following patch moves the Preferred/Favorite web shortcut selection checkbox into its own column to avoid confusion. The new column is marked as Preferred and also shows a tool tip message about is functionality. See the screenshot below. This addresses bugs 168223 and 218164. http://bugs.kde.org/show_bug.cgi?id=168223 http://bugs.kde.org/show_bug.cgi?id=218164 Diffs - kurifilter-plugins/ikws/ikwsopts.cpp f1cc481 kurifilter-plugins/ikws/ikwsopts_p.h 9cfc12c kurifilter-plugins/ikws/ikwsopts_ui.ui 440c201 Diff: http://git.reviewboard.kde.org/r/104900/diff/ Testing --- Screenshots --- Preferred selection column http://git.reviewboard.kde.org/r/104900/s/563/ Thanks, Dawit Alemayehu
Re: Review Request: Fix KShortcut to really allow the usage of multiple shortcuts
On May 9, 2012, 8:53 p.m., Michael Jansen wrote: Hate to break the news to you after all that work. But the exact same thing you have achieved with this patch could be achieved by using a QAction for the missing Shortcut. Or a KAction for that matter as long as it is not part of the KActionCollection. There is only one reason to use KAction. The Shortcuts Editor. And KXMLwhatever. And global shortcuts. So make that there are only three reasons to use KAction. None of them know how to handle a KAction with three Shortcuts set. So your patch has not (yet?) achieved anything you could not gain by just using a hidden unconfigurable second Q(K)Action. So i would say it does not make sense to apply it in its current form. Unless you are willing to go all the way which you only should do after finding out what the frameworks branchs does to kaction. So you effort is not thrown away in the near / middle future. Mike Well.. that sucks! Anyway, this patch is the first part. It doesn't break any backwards compatibility and simply allows apps that want to use more shortcuts to freely use them. The second patch would be against Dolphin to allow some more shortcuts. I already have the dolphin patch ready. After that the next patch would be to get the Shortcut Editor to support this as well. I don't have a patch for that, yet! From a quick look at KShortcut in frameworks it seems to be just the same as the current 4.8 branch. Just a bunch of TODO items for the constructors to use QShortcut. KShortcut doesn't seem to be going away. I would like to push this one and fix dolphin and the shortcut editor if i'm allowed to. If that's not oke, then please do tell me how to fix this the proper way. This is what i would like to start doing if it's not oke to push: Step 1. KAction to inherit QAction and add the global shortcut stuff. Perhaps some more stuff. Step 2. Rewrite KShortcut to inherit QShortcut and ONLY have the additional option for global shortcuts. Step 3. Adjust the Shortcut Editor to use the new structure. Step 4. Deprecate most of KAction in the 4.8 kdelibs branch. Step 5. Deprecate all of KShortcut except the global related stuff Your input would be welcome. -- little braindump -- It would be very nice to have a KDE KInputShortcut class in which you can mix shortcuts from various input devices. For example: KInputShortcut(Qt::CTRL + Qt::LeftButton) .. That would really add something useful! - Mark --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104801/#review13624 --- On May 9, 2012, 6:21 p.m., Mark Gaiser wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104801/ --- (Updated May 9, 2012, 6:21 p.m.) Review request for kdelibs. Description --- So i was trying to fix this bug: https://bugs.kde.org/show_bug.cgi?id=181531 That only asked for one more shortcut. That issue seems to be a little more complicated than it looks. Till this point KActions could only have a Primary and a Alternate shortcut. 2 in total which is - in some situations - not enough. I fixed this by roughly restructuring nearly all of the KShortcut cpp file. The only thing i'm not quite sure about is how KShortcut KAction::shortcut(ShortcutTypes type) const looks right now.. If anyone has some clarification on that one..? Diffs - kdeui/actions/kaction.h d877554 kdeui/actions/kaction.cpp 309cf82 kdeui/shortcuts/kshortcut.h c720830 kdeui/shortcuts/kshortcut.cpp e307ab0 Diff: http://git.reviewboard.kde.org/r/104801/diff/ Testing --- I tested this by adding the missing bindings for Dolphin's back/forward and it seems to be working just fine. I can use all available shortcuts. Thanks, Mark Gaiser
Re: Review Request: Move the Preferred Web shorcut selection checkbox into its own column
On May 9, 2012, 9:34 p.m., Andrea Diamantini wrote: Ship It! I like it. And code changes seem easy and safe. - Andrea --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104900/#review13627 --- On May 9, 2012, 8:35 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104900/ --- (Updated May 9, 2012, 8:35 p.m.) Review request for KDE Runtime. Description --- The following patch moves the Preferred/Favorite web shortcut selection checkbox into its own column to avoid confusion. The new column is marked as Preferred and also shows a tool tip message about is functionality. See the screenshot below. This addresses bugs 168223 and 218164. http://bugs.kde.org/show_bug.cgi?id=168223 http://bugs.kde.org/show_bug.cgi?id=218164 Diffs - kurifilter-plugins/ikws/ikwsopts.cpp f1cc481 kurifilter-plugins/ikws/ikwsopts_p.h 9cfc12c kurifilter-plugins/ikws/ikwsopts_ui.ui 440c201 Diff: http://git.reviewboard.kde.org/r/104900/diff/ Testing --- Screenshots --- Preferred selection column http://git.reviewboard.kde.org/r/104900/s/563/ Thanks, Dawit Alemayehu
Re: Review Request: Move the Preferred Web shorcut selection checkbox into its own column
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104900/#review13631 --- This review has been submitted with commit 0707b07d3afb14870d5c31149238e0f32e0d1187 by Dawit Alemayehu to branch master. - Commit Hook On May 9, 2012, 8:35 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104900/ --- (Updated May 9, 2012, 8:35 p.m.) Review request for KDE Runtime. Description --- The following patch moves the Preferred/Favorite web shortcut selection checkbox into its own column to avoid confusion. The new column is marked as Preferred and also shows a tool tip message about is functionality. See the screenshot below. This addresses bugs 168223 and 218164. http://bugs.kde.org/show_bug.cgi?id=168223 http://bugs.kde.org/show_bug.cgi?id=218164 Diffs - kurifilter-plugins/ikws/ikwsopts.cpp f1cc481 kurifilter-plugins/ikws/ikwsopts_p.h 9cfc12c kurifilter-plugins/ikws/ikwsopts_ui.ui 440c201 Diff: http://git.reviewboard.kde.org/r/104900/diff/ Testing --- Screenshots --- Preferred selection column http://git.reviewboard.kde.org/r/104900/s/563/ Thanks, Dawit Alemayehu
Re: Review Request: Plasma Components buttons: first focus, than emit clicked() signal
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104893/#review13632 --- This review has been submitted with commit fdb3ec834ee6912c82cdc436f614b8f7fb4fe8a5 by Sebastian Gottfried to branch master. - Commit Hook On May 9, 2012, 4:41 p.m., Sebastian Gottfried wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104893/ --- (Updated May 9, 2012, 4:41 p.m.) Review request for KDE Runtime. Description --- Otherwise a client wanting to give another QML component the focus in reaction to a clicked button has no chance doing so because the button will steal the focus again right after the event hander has finished executing. Diffs - plasma/declarativeimports/plasmacomponents/qml/Button.qml 6aab1b2 plasma/declarativeimports/plasmacomponents/qml/ToolButton.qml 00ffa4c Diff: http://git.reviewboard.kde.org/r/104893/diff/ Testing --- Tested the behaviour in ktouch/next, works fine. Thanks, Sebastian Gottfried
