Re: Review Request: Plasma Components buttons: first focus, than emit clicked() signal

[Commit Hook]

---
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
> 
>

Re: Review Request: Move the Preferred Web shorcut selection checkbox into its own column

[Commit Hook]

---
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: Move the Preferred Web shorcut selection checkbox into its own column

[Andrea Diamantini]

> 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: Fix KShortcut to really allow the usage of multiple shortcuts

[Mark Gaiser]

> 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

[Andrea Diamantini]

---
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: Plasma Components buttons: first focus, than emit clicked() signal

[Sebastian Kügler]

---
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: Fix KShortcut to really allow the usage of multiple shortcuts

[Michael Jansen]

---
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
> 
>

Review Request: Move the Preferred Web shorcut selection checkbox into its own column

[Dawit Alemayehu]

---
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: Added option for disabling the offer to save website passwords in Konqueror

[Dawit Alemayehu]

> On April 26, 2012, 5:09 p.m., Albert Astals Cid wrote:
> > If i understand you correctly you are suggesting to create a bug (option 
> > that does nothing)?
> > 
> > Doesn't make much sense.
> 
> Dawit Alemayehu wrote:
> Huh ? I do not follow. By "option that does nothing" you mean this change 
> by itself does nothing that is correct. But that is true of every option in 
> that dialog as well. Moreover, it is a well known fact that you cannot post a 
> patch for different components in reviewboard. Anyhow, the option will most 
> definitely be used by kwebkitpart. Whether or not some body implements 
> support for it in khtml is a question I cannot answer. That is what I meant.
> 
> David Faure wrote:
> Do you have the kwebkitpart patch ready?
> I suppose it'll be easy to "apply" to khtml as well.
> 
> Albert Astals Cid wrote:
> You are suggesting to add an option that does nothing in our default html 
> rendering engine. That is adding a bug. The fact that you know it and don't 
> care about it makes me sad.

@David: Yes I have a patch for kwebkitpart, but unlike in khtml adding support 
for this in kwebkitpart required a very small change in one place in addition 
to adding code to read the config option itself in webkitettings.{h|cpp}. That 
does not seem to be the case in khtml. It is a little bit more invovled.

@Albert: Really ?? So how exactly is another browser engine supposed to provide 
configuration option to the user if it is supposed to be embedded into 
Konqueror ?  Why would a developer working on a separate browsing engine be 
forced to implement any functionality into khtml ? Would that requirement apply 
the other way around ? The last I checked, this is a konqueror setting. Whether 
a specific browser engine supports it or not is up to that browser engine.Just 
for the record I almost always go out of my way to implement things in khtml ; 
especially when I factor out features that are common to both engines. In this 
case, I neither have the time nor a complete grasp of how the KWallet 
integration works in khtml. As I stated above the change is not in a single 
isolated location like it is in kwebkitpart. Feel free to do a grep in khtml 
and see for yourself. I can always add the set/get functions to access the 
option in KHTMLSettings, but what use would that be if it is not implemented. 

Anyhow, I digress. If there are objections, I will simply move this feature 
into the webkit config module I will have to create down the road.


- Dawit


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104631/#review12934
---


On April 26, 2012, 3:48 a.m., Dawit Alemayehu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104631/
> ---
> 
> (Updated April 26, 2012, 3:48 a.m.)
> 
> 
> Review request for KDE Base Apps, kdelibs and David Faure.
> 
> 
> Description
> ---
> 
> A patch to make that provides an option to disable the "offer to save website 
> passwords". Note that for this patch to take effect this option will have to 
> be used at at the browser engine level. Those patches are separate to this 
> one. This is just the GUI configuration.
> 
> 
> Diffs
> -
> 
>   konqueror/settings/konqhtml/htmlopts.h 69f36d8 
>   konqueror/settings/konqhtml/htmlopts.cpp e5d6deb 
> 
> Diff: http://git.reviewboard.kde.org/r/104631/diff/
> 
> 
> Testing
> ---
> 
> 
> Screenshots
> ---
> 
> Option for disabling KWallet integration
>   http://git.reviewboard.kde.org/r/104631/s/544/
> 
> 
> Thanks,
> 
> Dawit Alemayehu
> 
>

Re: Review Request: dataprotocol: make charset recoding work

[Commit Hook]

---
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
> 
>

Re: Review Request: Plasma Components: TextField and TextArea: Show placeholders even if item has the focus

[Mark Gaiser]

---
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: Plasma Components buttons: first focus, than emit clicked() signal

[Laszlo Papp]

---
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
> 
>

Review Request: Plasma Components: TextField and TextArea: Show placeholders even if item has the focus

[Sebastian Gottfried]

---
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: dataprotocol: simplify helper code

[Commit Hook]

---
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
> 
>

Re: Review Request: Use the focus proxy, if present, as parent of KonqPopupMenu

[Dawit Alemayehu]

> 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
> 
>

KDE 4.9 feature plan

[Anne-Marie Mahfouf]

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: dataprotocol: make charset recoding work

[Rolf Eike Beer]

> On May 9, 2012, 12:04 p.m., David Faure wrote:
> > kio/kio/dataprotocol.cpp, line 71
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> 
>

Re: Re: Package Dependcies List on Techbase

[Rick Stockton]

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: simplify helper code

[David Faure]

---
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


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


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: Review Request: dataprotocol: make charset recoding work

[David Faure]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104874/#review13603
---



kio/kio/dataprotocol.cpp


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


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


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: Use the focus proxy, if present, as parent of KonqPopupMenu

[David Faure]

---
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
> 
>

Review Request: Use the focus proxy, if present, as parent of KonqPopupMenu

[Dawit Alemayehu]

---
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