Review Request 123161: Add copy and paste support for calculator widget

2015-03-27 Thread Bernhard Friedreich 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123161/
---

Review request for Plasma.


Bugs: 318221
http://bugs.kde.org/show_bug.cgi?id=318221


Repository: kdeplasma-addons


Description
---

The current displayed value from the textfield can be copied.
Content from the clipboard can be pasted but only if the content
is a valid number

BUG: 318221


Diffs
-

  applets/calculator/package/contents/ui/calculator.qml 
23f74bd1ac7de6b7f4519677bd96aa351a91cff1 

Diff: https://git.reviewboard.kde.org/r/123161/diff/


Testing
---

Copying:
*) Tried copying entered numbers and results - works for me :)

Pasting:
*) Pasting "asdf" leads to nothing being pasted - correct
*) Pasting "5" pasts 5 - replaces the currently entered text with the pasted 
content (this could be worked around - should it append?)
*) Pasting "= 5 + 3" passes through validation - no idea why yet - this should 
be possible I guess?

Comments welcome :) This is my second patch so hopefully my work isn't complete 
garbage :P


Thanks,

Bernhard Friedreich

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 123161: Add copy and paste support for calculator widget

2015-03-27 Thread David Edmundson 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123161/#review78129
---



applets/calculator/package/contents/ui/calculator.qml (line 95)


avoid guessing what the shortcut is.

Otherwise if someone maps it to alt in their settings it won't work. Or if 
someone has a dvorak layout it won't be c and v.

Use

if event.matches(StandardKey.Copy)
http://doc.qt.io/qt-5/qml-qtquick-keyevent.html#matches-method

internally this will ask our QPT plugin which loads things from a KDE 
config file.



applets/calculator/package/contents/ui/calculator.qml (line 220)


I'm not convinced this is going to work.

You'll update the display, but all the handling of storing what number is 
entered handles in the key press events.

Can you try pasting "5" then typing '+' and '1'
and see what happens.

I think it will then show 1, not 6.


- David Edmundson


On March 27, 2015, 11:21 p.m., Bernhard Friedreich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123161/
> ---
> 
> (Updated March 27, 2015, 11:21 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Bugs: 318221
> http://bugs.kde.org/show_bug.cgi?id=318221
> 
> 
> Repository: kdeplasma-addons
> 
> 
> Description
> ---
> 
> The current displayed value from the textfield can be copied.
> Content from the clipboard can be pasted but only if the content
> is a valid number
> 
> BUG: 318221
> 
> 
> Diffs
> -
> 
>   applets/calculator/package/contents/ui/calculator.qml 
> 23f74bd1ac7de6b7f4519677bd96aa351a91cff1 
> 
> Diff: https://git.reviewboard.kde.org/r/123161/diff/
> 
> 
> Testing
> ---
> 
> Copying:
> *) Tried copying entered numbers and results - works for me :)
> 
> Pasting:
> *) Pasting "asdf" leads to nothing being pasted - correct
> *) Pasting "5" pasts 5 - replaces the currently entered text with the pasted 
> content (this could be worked around - should it append?)
> *) Pasting "= 5 + 3" passes through validation - no idea why yet - this 
> should be possible I guess?
> 
> Comments welcome :) This is my second patch so hopefully my work isn't 
> complete garbage :P
> 
> 
> Thanks,
> 
> Bernhard Friedreich
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 123161: Add copy and paste support for calculator widget

2015-03-28 Thread Bernhard Friedreich 


> On März 27, 2015, 11:46 nachm., David Edmundson wrote:
> > applets/calculator/package/contents/ui/calculator.qml, line 220
> > 
> >
> > I'm not convinced this is going to work.
> > 
> > You'll update the display, but all the handling of storing what number 
> > is entered handles in the key press events.
> > 
> > Can you try pasting "5" then typing '+' and '1'
> > and see what happens.
> > 
> > I think it will then show 1, not 6.

you are right - this doesn't work.. slipped my testing somehow..
should I iterate over every character of the clipboard and call displayNumber 
for each? Would reuse the existing codepath..


> On März 27, 2015, 11:46 nachm., David Edmundson wrote:
> > applets/calculator/package/contents/ui/calculator.qml, line 95
> > 
> >
> > avoid guessing what the shortcut is.
> > 
> > Otherwise if someone maps it to alt in their settings it won't work. Or 
> > if someone has a dvorak layout it won't be c and v.
> > 
> > 
> > Use
> > 
> > if event.matches(StandardKey.Copy)
> > http://doc.qt.io/qt-5/qml-qtquick-keyevent.html#matches-method
> > 
> > internally this will ask our QPT plugin which loads things from a KDE 
> > config file.

I had already found this before but the clipboard plasmoid was doing it like 
this so I thought this was the prefered way - makes sense and is more readable. 
Thanks :)


- Bernhard


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123161/#review78129
---


On März 27, 2015, 11:21 nachm., Bernhard Friedreich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123161/
> ---
> 
> (Updated März 27, 2015, 11:21 nachm.)
> 
> 
> Review request for Plasma.
> 
> 
> Bugs: 318221
> http://bugs.kde.org/show_bug.cgi?id=318221
> 
> 
> Repository: kdeplasma-addons
> 
> 
> Description
> ---
> 
> The current displayed value from the textfield can be copied.
> Content from the clipboard can be pasted but only if the content
> is a valid number
> 
> BUG: 318221
> 
> 
> Diffs
> -
> 
>   applets/calculator/package/contents/ui/calculator.qml 
> 23f74bd1ac7de6b7f4519677bd96aa351a91cff1 
> 
> Diff: https://git.reviewboard.kde.org/r/123161/diff/
> 
> 
> Testing
> ---
> 
> Copying:
> *) Tried copying entered numbers and results - works for me :)
> 
> Pasting:
> *) Pasting "asdf" leads to nothing being pasted - correct
> *) Pasting "5" pasts 5 - replaces the currently entered text with the pasted 
> content (this could be worked around - should it append?)
> *) Pasting "= 5 + 3" passes through validation - no idea why yet - this 
> should be possible I guess?
> 
> Comments welcome :) This is my second patch so hopefully my work isn't 
> complete garbage :P
> 
> 
> Thanks,
> 
> Bernhard Friedreich
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 123161: Add copy and paste support for calculator widget

2015-03-28 Thread David Edmundson 


> On March 27, 2015, 11:46 p.m., David Edmundson wrote:
> > applets/calculator/package/contents/ui/calculator.qml, line 220
> > 
> >
> > I'm not convinced this is going to work.
> > 
> > You'll update the display, but all the handling of storing what number 
> > is entered handles in the key press events.
> > 
> > Can you try pasting "5" then typing '+' and '1'
> > and see what happens.
> > 
> > I think it will then show 1, not 6.
> 
> Bernhard Friedreich wrote:
> you are right - this doesn't work.. slipped my testing somehow..
> should I iterate over every character of the clipboard and call 
> displayNumber for each? Would reuse the existing codepath..

I think it's our only option.


> On March 27, 2015, 11:46 p.m., David Edmundson wrote:
> > applets/calculator/package/contents/ui/calculator.qml, line 95
> > 
> >
> > avoid guessing what the shortcut is.
> > 
> > Otherwise if someone maps it to alt in their settings it won't work. Or 
> > if someone has a dvorak layout it won't be c and v.
> > 
> > 
> > Use
> > 
> > if event.matches(StandardKey.Copy)
> > http://doc.qt.io/qt-5/qml-qtquick-keyevent.html#matches-method
> > 
> > internally this will ask our QPT plugin which loads things from a KDE 
> > config file.
> 
> Bernhard Friedreich wrote:
> I had already found this before but the clipboard plasmoid was doing it 
> like this so I thought this was the prefered way - makes sense and is more 
> readable. Thanks :)

Urgh. It won't for much longer.
Thanks for pointing that out


- David


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123161/#review78129
---


On March 27, 2015, 11:21 p.m., Bernhard Friedreich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123161/
> ---
> 
> (Updated March 27, 2015, 11:21 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Bugs: 318221
> http://bugs.kde.org/show_bug.cgi?id=318221
> 
> 
> Repository: kdeplasma-addons
> 
> 
> Description
> ---
> 
> The current displayed value from the textfield can be copied.
> Content from the clipboard can be pasted but only if the content
> is a valid number
> 
> BUG: 318221
> 
> 
> Diffs
> -
> 
>   applets/calculator/package/contents/ui/calculator.qml 
> 23f74bd1ac7de6b7f4519677bd96aa351a91cff1 
> 
> Diff: https://git.reviewboard.kde.org/r/123161/diff/
> 
> 
> Testing
> ---
> 
> Copying:
> *) Tried copying entered numbers and results - works for me :)
> 
> Pasting:
> *) Pasting "asdf" leads to nothing being pasted - correct
> *) Pasting "5" pasts 5 - replaces the currently entered text with the pasted 
> content (this could be worked around - should it append?)
> *) Pasting "= 5 + 3" passes through validation - no idea why yet - this 
> should be possible I guess?
> 
> Comments welcome :) This is my second patch so hopefully my work isn't 
> complete garbage :P
> 
> 
> Thanks,
> 
> Bernhard Friedreich
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 123161: Add copy and paste support for calculator widget

2015-03-28 Thread Bernhard Friedreich 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123161/
---

(Updated März 28, 2015, 2:45 nachm.)


Review request for Plasma.


Changes
---

Fixed pasting and implemented suggestions.


Bugs: 318221
http://bugs.kde.org/show_bug.cgi?id=318221


Repository: kdeplasma-addons


Description
---

The current displayed value from the textfield can be copied.
Content from the clipboard can be pasted but only if the content
is a valid number

BUG: 318221


Diffs (updated)
-

  applets/calculator/package/contents/ui/calculator.qml 
23f74bd1ac7de6b7f4519677bd96aa351a91cff1 

Diff: https://git.reviewboard.kde.org/r/123161/diff/


Testing (updated)
---

Copying:
*) Tried copying entered numbers and results - works for me :)

Pasting:
*) Pasting "asdf" leads to nothing being pasted - correct
*) Pasting "5" appends the number to the currently entered input or replaces 
the current number in case an operator has been pressed before

Only pasting doubles is possible - no signs or full math expressions are 
supported. 
It looks like the plasmoid doesn't supports signed numbers in general (e.g. 5 - 
-5 doesnt result in 10)

Comments welcome :) This is my second patch so hopefully my work isn't complete 
garbage :P


Thanks,

Bernhard Friedreich

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 123161: Add copy and paste support for calculator widget

2015-03-28 Thread David Edmundson 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123161/#review78150
---

Ship it!


Ship It!

- David Edmundson


On March 28, 2015, 2:45 p.m., Bernhard Friedreich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123161/
> ---
> 
> (Updated March 28, 2015, 2:45 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Bugs: 318221
> http://bugs.kde.org/show_bug.cgi?id=318221
> 
> 
> Repository: kdeplasma-addons
> 
> 
> Description
> ---
> 
> The current displayed value from the textfield can be copied.
> Content from the clipboard can be pasted but only if the content
> is a valid number
> 
> BUG: 318221
> 
> 
> Diffs
> -
> 
>   applets/calculator/package/contents/ui/calculator.qml 
> 23f74bd1ac7de6b7f4519677bd96aa351a91cff1 
> 
> Diff: https://git.reviewboard.kde.org/r/123161/diff/
> 
> 
> Testing
> ---
> 
> Copying:
> *) Tried copying entered numbers and results - works for me :)
> 
> Pasting:
> *) Pasting "asdf" leads to nothing being pasted - correct
> *) Pasting "5" appends the number to the currently entered input or replaces 
> the current number in case an operator has been pressed before
> 
> Only pasting doubles is possible - no signs or full math expressions are 
> supported. 
> It looks like the plasmoid doesn't supports signed numbers in general (e.g. 5 
> - -5 doesnt result in 10)
> 
> Comments welcome :) This is my second patch so hopefully my work isn't 
> complete garbage :P
> 
> 
> Thanks,
> 
> Bernhard Friedreich
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 123161: Add copy and paste support for calculator widget

2015-03-28 Thread Bernhard Friedreich 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123161/
---

(Updated March 28, 2015, 3:18 p.m.)


Status
--

This change has been marked as submitted.


Review request for Plasma.


Changes
---

Submitted with commit 16cad20c28e09109844f1e34ae87c71ae8259350 by David 
Edmundson on behalf of Bernhard Friedreich to branch master.


Bugs: 318221
http://bugs.kde.org/show_bug.cgi?id=318221


Repository: kdeplasma-addons


Description
---

The current displayed value from the textfield can be copied.
Content from the clipboard can be pasted but only if the content
is a valid number

BUG: 318221


Diffs
-

  applets/calculator/package/contents/ui/calculator.qml 
23f74bd1ac7de6b7f4519677bd96aa351a91cff1 

Diff: https://git.reviewboard.kde.org/r/123161/diff/


Testing
---

Copying:
*) Tried copying entered numbers and results - works for me :)

Pasting:
*) Pasting "asdf" leads to nothing being pasted - correct
*) Pasting "5" appends the number to the currently entered input or replaces 
the current number in case an operator has been pressed before

Only pasting doubles is possible - no signs or full math expressions are 
supported. 
It looks like the plasmoid doesn't supports signed numbers in general (e.g. 5 - 
-5 doesnt result in 10)

Comments welcome :) This is my second patch so hopefully my work isn't complete 
garbage :P


Thanks,

Bernhard Friedreich

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel