Re: Review Request: replace old kickoff with kickoff-qml

2012-09-19 Thread Martin Gräßlin 

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


wow, what a large patch :-) I still see a lot of code written by me so I can 
hardly review it. I have just looked a little bit over it, but need to properly 
test.

I'm all for getting this into 4.10, it's a really important part to go to QML.


plasma/desktop/applets/kickoff/core/models.h


that looks kind of wrong. I would swap LastDataRole with GroupNameRole and 
make LastDataRole = GroupNameRole



plasma/desktop/applets/kickoff/package/contents/ui/ApplicationsView.qml


depending on how much you changed in the qml files, please also add 
yourself to the copyright headers


- Martin Gräßlin


On Sept. 15, 2012, 4:41 p.m., Greg T wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106448/
> ---
> 
> (Updated Sept. 15, 2012, 4:41 p.m.)
> 
> 
> Review request for Plasma and Martin Gräßlin.
> 
> 
> Description
> ---
> 
> I think it's time now to get the new kickoff into master so we can polish it 
> for KDE 4.10. the qml plasmoid is still using the old model code (though 
> slightly adjusted). This could cause issues for the c++ menu launcher 
> "simpleapplet"...I'm not using it but actually it doesn't even compile. 
> Probably would be better to port it to qml as well...opinions?
> 
> The qml code resides in the package dir. It's a pure qml widget with c++ 
> model extensions...obviously :)
> 
> Add to Panel/Desktop is still not supported but overall I think we have 
> reached feature parity with kickoff c++
> 
> I'm also worried about the upgrade path from kickoff-c++ to kickoff-qml. How 
> can we ensure a smooth transition for devs and users?
> 
> 
> Diffs
> -
> 
>   ksplash/ksplashqml/SplashWindow.cpp 
> 94e6dedfddcac20516686a7a3b30651d48bc26e7 
>   plasma/desktop/applets/kickoff/CMakeLists.txt 
> 4b0d32a9d0e7d46d05c4efdef9dc39fd653cc2f9 
>   plasma/desktop/applets/kickoff/core/CMakeLists.txt PRE-CREATION 
>   plasma/desktop/applets/kickoff/core/applicationmodel.h 
> f0f8872255956321292151cdd82326cdf88c5508 
>   plasma/desktop/applets/kickoff/core/applicationmodel.cpp 
> 74b2595ba67de1cdd2f93d3e9e7c2eebd68f6df2 
>   plasma/desktop/applets/kickoff/core/favoritesmodel.h 
> 8ee3e9a9eb16780131d59b150b50641e5a03a34c 
>   plasma/desktop/applets/kickoff/core/favoritesmodel.cpp 
> 14e35cdced82384f0cdf86caec9a4045bfaf2912 
>   plasma/desktop/applets/kickoff/core/itemhandlers.h 
> ec72cbe51b6e2da604ba6eba96f9e6f3f5935f67 
>   plasma/desktop/applets/kickoff/core/itemhandlers.cpp 
> 4e83c37588af1ebab331082e2eaccb40a0f8155c 
>   plasma/desktop/applets/kickoff/core/kickoffabstractmodel.cpp 
> 7e2e64d22e9e274ffe3d37fdd0ac2c33a622ea3a 
>   plasma/desktop/applets/kickoff/core/kickoffmodel.cpp 
> 8149cac20ce8ab246d8f484ca7567fc6e32d548c 
>   plasma/desktop/applets/kickoff/core/kickoffplugin.h PRE-CREATION 
>   plasma/desktop/applets/kickoff/core/kickoffplugin.cpp PRE-CREATION 
>   plasma/desktop/applets/kickoff/core/kickoffproxymodel.cpp 
> f92bca971e0f9fcb644cadab6aa39a3e36291c00 
>   plasma/desktop/applets/kickoff/core/krunnermodel.h 
> 93a8b152a673eb6233727a82eefd70739ffc5a0f 
>   plasma/desktop/applets/kickoff/core/krunnermodel.cpp 
> 452ebbe81311f8e3e95b5eda5fb9217344852d06 
>   plasma/desktop/applets/kickoff/core/leavemodel.h 
> 0676fb9358bdfd5e3cffce7eb3a0ea5e4ff70989 
>   plasma/desktop/applets/kickoff/core/leavemodel.cpp 
> 31467acc6654decb2800cf9a5acc398ccaaeccc7 
>   plasma/desktop/applets/kickoff/core/models.h 
> 3332ba9608808b353c32d96c37b84ddd82aabddf 
>   plasma/desktop/applets/kickoff/core/models.cpp 
> c787df6e2f2c7c88ff97c64c7cd7640cce32365b 
>   plasma/desktop/applets/kickoff/core/qmldir PRE-CREATION 
>   plasma/desktop/applets/kickoff/core/recentapplications.h 
> a0feddca1cebbeb556623216bcc6c5c30e13a2a4 
>   plasma/desktop/applets/kickoff/core/recentapplications.cpp 
> 3e0538958564ae690e41791bdb5af76fa2ca9a8f 
>   plasma/desktop/applets/kickoff/core/recentlyusedmodel.h 
> 841eb2b77aee778a85c76eafa61d38016f6ade58 
>   plasma/desktop/applets/kickoff/core/recentlyusedmodel.cpp 
> 2762d6d63a7b0592a7e87cd99603cc7c418292c5 
>   plasma/desktop/applets/kickoff/core/systemmodel.h 
> b04a7871883edd5ea7281d9853ec9203ce019894 
>   plasma/desktop/applets/kickoff/core/systemmodel.cpp 
> cb9bf650bab36c6415d37db795e766b743d5e25d 
>   plasma/desktop/applets/kickoff/core/urlitemlauncher.h 
> 26b638fc02f42505e29857b5c18736e6778a580e 
>   plasma/desktop/applets/kickoff/core/urlitemlauncher.cpp 
> 75bbfb5a5c6df837e7e56de501156b2ca12ed6d7 
>   plasma/desktop/applets/kickoff/package/contents/confi

Re: Review Request: Fix setting the size of the calendar of clocks

2012-09-19 Thread Ralf Jung 


> On Sept. 18, 2012, 10:13 p.m., Sebastian Kügler wrote:
> > Patch looks good now, thanks a lot!
> > 
> > Please commit it to KDE/4.9 and master.
> 
> Sebastian Kügler wrote:
> Ah, crap. I missed your question about the possible BIC issue. Can 
> someone answer this question?
> 
> """
> Is it okay to remove a no longer needed overload of a virtual function 
> from a class in kde-workspace/libs, or is there some BC constraint?
> """
> (the patch changes updatePreferredSize() to updateSize())
> 
> Please hold off pushing the patch for now, we really don't want to ship 
> any BIC issues into a stable branch. :/
> 
> Marco Martin wrote:
> yes, that patch breaks bc.
> however a library in kde-workspace has no promise of bc.
> only one thing, shouldn't break released software, so it's fine to commit 
> to master but not backporting to 4.9

Actually, the patch presented here does not break BC. This function I renamed 
is part of CalendarPrivate and therefore can not be used from the outside.
The BC breakage would (possibly) be introduced by removing 
Calendar::resizeEvent, which is why I just stubbed it out to call the 
superclass function.


- Ralf


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


On Sept. 18, 2012, 5:40 p.m., Ralf Jung wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106441/
> ---
> 
> (Updated Sept. 18, 2012, 5:40 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Description
> ---
> 
> Currently, the size of the calendar opened when clicking a clock is sometimes 
> set incorrectly, see the bug I linked to this review request. This patch 
> fixes the bug by setting a proper minimal size on the calendar widget and 
> updating the displayed events after configuration changes.
> 
> 
> This addresses bug 306762.
> http://bugs.kde.org/show_bug.cgi?id=306762
> 
> 
> Diffs
> -
> 
>   libs/plasmaclock/calendar.cpp 7ea70c2 
> 
> Diff: http://git.reviewboard.kde.org/r/106441/diff/
> 
> 
> Testing
> ---
> 
> I verified that the bug is fixed - everything behaved as expected.
> 
> 
> Thanks,
> 
> Ralf Jung
> 
>

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

Re: Review Request: make plasmate able to load a project in the second time

2012-09-19 Thread Giorgos Tsiapaliwkas 


> On Sept. 18, 2012, 11:32 p.m., Aaron J. Seigo wrote:
> > mainwindow.cpp, line 488
> > 
> >
> > this looks incorrect; probably what is missing is an "|| !m_part"

it works with !m_part


- Giorgos


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


On Sept. 17, 2012, 11:20 a.m., Giorgos Tsiapaliwkas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106479/
> ---
> 
> (Updated Sept. 17, 2012, 11:20 a.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Description
> ---
> 
> How to reproduce the issue,
> 
> 1. open a project with plasmate
> 2. go File->close project
> 3. open the project again.
> A kmessagebox will appear with the error.
> 
> 
> Diffs
> -
> 
>   mainwindow.cpp b84da4a 
> 
> Diff: http://git.reviewboard.kde.org/r/106479/diff/
> 
> 
> Testing
> ---
> 
> I have tested the patch and I haven't seen any issues
> 
> 
> Thanks,
> 
> Giorgos Tsiapaliwkas
> 
>

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

Re: Review Request: make plasmate able to load a project in the second time

2012-09-19 Thread Giorgos Tsiapaliwkas 

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

(Updated Sept. 19, 2012, 11:03 a.m.)


Review request for Plasma.


Changes
---

update diff based on Aaron's review


Description
---

How to reproduce the issue,

1. open a project with plasmate
2. go File->close project
3. open the project again.
A kmessagebox will appear with the error.


Diffs (updated)
-

  mainwindow.cpp b84da4a 

Diff: http://git.reviewboard.kde.org/r/106479/diff/


Testing
---

I have tested the patch and I haven't seen any issues


Thanks,

Giorgos Tsiapaliwkas

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

Re: Review Request: hide the konsole on the close project

2012-09-19 Thread Commit Hook 

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


This review has been submitted with commit 
97afaff360bf2c0005346b15fd219f7ae1c09ab3 by Giorgos Tsiapaliokas to branch 
master.

- Commit Hook


On Sept. 17, 2012, 11:17 a.m., Giorgos Tsiapaliwkas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106478/
> ---
> 
> (Updated Sept. 17, 2012, 11:17 a.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Description
> ---
> 
> How to reproduce,
> 
> 1. open a project with plasmate.
> 2. go File->close Project
> 3. look at the startpage, the konsole is still there
> 
> Also in this patch I did s/m_konsole/m_konsoleWidget. The rest of the
> widget use the naming m_fooWidget.
> 
> 
> Diffs
> -
> 
>   mainwindow.h 8005d26 
>   mainwindow.cpp b84da4a 
> 
> Diff: http://git.reviewboard.kde.org/r/106478/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Giorgos Tsiapaliwkas
> 
>

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

Re: Review Request: The preview action shouldn't be visible all the time

2012-09-19 Thread Giorgos Tsiapaliwkas 


> On Sept. 17, 2012, 4:13 p.m., Sebastian Kügler wrote:
> > mainwindow.cpp, line 975
> > 
> >
> > Do we setVisible(true) it when we change package?
> > 
> > I don't see this in the code, but haven't tried. Something to check...

yes you have right


- Giorgos


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


On Sept. 17, 2012, 11:14 a.m., Giorgos Tsiapaliwkas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106477/
> ---
> 
> (Updated Sept. 17, 2012, 11:14 a.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Description
> ---
> 
> This patches fixes the above bugs
> 
> 
> This addresses bugs 305866 and 305867.
> http://bugs.kde.org/show_bug.cgi?id=305866
> http://bugs.kde.org/show_bug.cgi?id=305867
> 
> 
> Diffs
> -
> 
>   mainwindow.cpp b84da4a 
> 
> Diff: http://git.reviewboard.kde.org/r/106477/diff/
> 
> 
> Testing
> ---
> 
> works
> 
> 
> Thanks,
> 
> Giorgos Tsiapaliwkas
> 
>

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

Re: Review Request: The preview action shouldn't be visible all the time

2012-09-19 Thread Giorgos Tsiapaliwkas 

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

(Updated Sept. 19, 2012, 11:13 a.m.)


Review request for Plasma.


Changes
---

update the review based on sebas review


Description
---

This patches fixes the above bugs


This addresses bugs 305866 and 305867.
http://bugs.kde.org/show_bug.cgi?id=305866
http://bugs.kde.org/show_bug.cgi?id=305867


Diffs (updated)
-

  mainwindow.cpp 1023f29 

Diff: http://git.reviewboard.kde.org/r/106477/diff/


Testing
---

works


Thanks,

Giorgos Tsiapaliwkas

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

Review Request: fix crash in runner due to bookmarks

2012-09-19 Thread Giorgos Tsiapaliwkas 

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

Review request for Plasma.


Description
---

krunner crashes due to 
kde-workspace/plasma/generic/runners/bookmarks/browsers/firefox.cpp 
(Firefox::teardown()).

This patch solves the issue.

Also I fixed the indentation and a mem leak.


Diffs
-

  plasma/generic/runners/bookmarks/browsers/firefox.cpp 4dc02e3 

Diff: http://git.reviewboard.kde.org/r/106504/diff/


Testing
---

since yesterday that I use the patch I haven't see any crashes


Thanks,

Giorgos Tsiapaliwkas

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

Re: Review Request: fix crash in runner due to bookmarks

2012-09-19 Thread Marco Gulino 

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


Why the "if(m_favicon)"? You could just delete m_favicon and set it to 0 (or 
better, NULL), as "delete NULL" is a noop.
Apart from this, I think it's ok :)


- Marco Gulino


On Sept. 19, 2012, 11:21 a.m., Giorgos Tsiapaliwkas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106504/
> ---
> 
> (Updated Sept. 19, 2012, 11:21 a.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Description
> ---
> 
> krunner crashes due to 
> kde-workspace/plasma/generic/runners/bookmarks/browsers/firefox.cpp 
> (Firefox::teardown()).
> 
> This patch solves the issue.
> 
> Also I fixed the indentation and a mem leak.
> 
> 
> Diffs
> -
> 
>   plasma/generic/runners/bookmarks/browsers/firefox.cpp 4dc02e3 
> 
> Diff: http://git.reviewboard.kde.org/r/106504/diff/
> 
> 
> Testing
> ---
> 
> since yesterday that I use the patch I haven't see any crashes
> 
> 
> Thanks,
> 
> Giorgos Tsiapaliwkas
> 
>

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

Re: Review Request: The preview action shouldn't be visible all the time

2012-09-19 Thread Sebastian Kügler 

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

Ship it!


Ship It!

- Sebastian Kügler


On Sept. 19, 2012, 11:13 a.m., Giorgos Tsiapaliwkas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106477/
> ---
> 
> (Updated Sept. 19, 2012, 11:13 a.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Description
> ---
> 
> This patches fixes the above bugs
> 
> 
> This addresses bugs 305866 and 305867.
> http://bugs.kde.org/show_bug.cgi?id=305866
> http://bugs.kde.org/show_bug.cgi?id=305867
> 
> 
> Diffs
> -
> 
>   mainwindow.cpp 1023f29 
> 
> Diff: http://git.reviewboard.kde.org/r/106477/diff/
> 
> 
> Testing
> ---
> 
> works
> 
> 
> Thanks,
> 
> Giorgos Tsiapaliwkas
> 
>

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

Re: Re: Status of QML Porting

2012-09-19 Thread Alex Fiestas 
Assigned the Picture Frame for myself, I was going to do the Calculator 
instead but I found a blogpost by Davide saying that he was going to do it.

So, last call... is anyone porting Picture Frame?

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

Re: Review Request: Make Sonnet::Speller::setLanguage threadsafe, fixing segfaults in krunner

2012-09-19 Thread Zack Rusin 


> On Sept. 11, 2012, 7:39 p.m., Zack Rusin wrote:
> > That's pretty bonkers. This class was never meant to be used like that. 
> > Holding an object in two threads is not going to work unless you make the 
> > entire communication synchronous effectively reducing all the 
> > multi-threading aspects to a "very complicated single thread". It just 
> > complicates this class.
> > The proper fix is to, instead of trying to synchronize one object owned by 
> > multiple threads, make the speller property of just the thread that does 
> > the spelling (or even just moveToThread it if you have to) and instead of 
> > calling setLanguage from another thread create a signal/slot combination 
> > between the parent thread and the speller thread and send a language change 
> > request by emitting a signal from the parent thread.
> 
> Simeon Bird wrote:
> I understand that this was not in the original design for the class; if 
> it were, the patch would not be necessary. However, the architecture of 
> krunner renders the scheme you describe extremely difficult and messy to 
> implement. 
> This is because krunner posts each set of input to a different thread, 
> and the input contains both the word to be spelt and the language. 
> 
> So, yes, we do actually need to be spelling multiple things in different 
> threads. Which means we have to be careful to not change the language from 
> under them. What you are suggesting won't work because all threads need to do 
> the spelling. I can create an extra thread that does it, and then have it run 
> an event loop fed from the helper threads, but that completely obviates the 
> reason krunner uses multiple threads in the first place, and is in addition 
> wildly over-complicated.
> 
> If you really don't like a mutex in this class, I can instead stick it 
> into the krunner function directly. Thus the thing you don't like would be 
> restricted to one obscure corner of plasma, rather than in kdelibs.

No, what I'm saying is that none of the objects in this framework was designed 
to be used by multiple threads. It's not a matter of missing functionality, 
you're simply using it wrong. Sonnet shouldn't have to be solving problems of 
thread syncing in krunner. I don't really know what "not to change the language 
from under them" means, neither do I understand why connecting a change 
language signal from some master thread to multiple spell checking threads 
wouldn't work, but the solution to the problem is not try to shoehorn a 
paradigm for which a framework was never intended, but to fix the application 
to use the framework properly. In the worst case you can always create your 
MTSpeller class in your plugin, use sonner::speller inside it and add whatever 
locking your heart desires to it.


- Zack


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


On Sept. 9, 2012, 10:06 p.m., Simeon Bird wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106242/
> ---
> 
> (Updated Sept. 9, 2012, 10:06 p.m.)
> 
> 
> Review request for kdelibs and Plasma.
> 
> 
> Description
> ---
> 
> Krunner's spellcheck plugin has been pretty broken since 
> bd291d21f096a714a171e7af3a534ba345ca5659 (about two years ago) because it 
> called Sonnet::Speller::setLanguage every time the spellchecker was invoked, 
> which was (very much) not thread-safe.
> 
> This patch makes Sonnet::Speller::setLanguage threadsafe by protecting all 
> access to the internal dict pointer using QReadWriteLock. 
> 
> A related review request is 106244, which adds more fixes to the spellcheck 
> feature.
> 
> 
> This addresses bugs 264779 and 303831.
> http://bugs.kde.org/show_bug.cgi?id=264779
> http://bugs.kde.org/show_bug.cgi?id=303831
> 
> 
> Diffs
> -
> 
>   kdecore/sonnet/speller.cpp b19e74d 
> 
> Diff: http://git.reviewboard.kde.org/r/106242/diff/
> 
> 
> Testing
> ---
> 
> Compiled, installed, used for a week or so, spellchecked a bunch of things.
> 
> 
> Thanks,
> 
> Simeon Bird
> 
>

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

Re: Review Request: Make Sonnet::Speller::setLanguage threadsafe, fixing segfaults in krunner

2012-09-19 Thread Simeon Bird 


> On Sept. 11, 2012, 7:39 p.m., Zack Rusin wrote:
> > That's pretty bonkers. This class was never meant to be used like that. 
> > Holding an object in two threads is not going to work unless you make the 
> > entire communication synchronous effectively reducing all the 
> > multi-threading aspects to a "very complicated single thread". It just 
> > complicates this class.
> > The proper fix is to, instead of trying to synchronize one object owned by 
> > multiple threads, make the speller property of just the thread that does 
> > the spelling (or even just moveToThread it if you have to) and instead of 
> > calling setLanguage from another thread create a signal/slot combination 
> > between the parent thread and the speller thread and send a language change 
> > request by emitting a signal from the parent thread.

I understand that this was not in the original design for the class; if it 
were, the patch would not be necessary. However, the architecture of krunner 
renders the scheme you describe extremely difficult and messy to implement. 
This is because krunner posts each set of input to a different thread, and the 
input contains both the word to be spelt and the language. 

So, yes, we do actually need to be spelling multiple things in different 
threads. Which means we have to be careful to not change the language from 
under them. What you are suggesting won't work because all threads need to do 
the spelling. I can create an extra thread that does it, and then have it run 
an event loop fed from the helper threads, but that completely obviates the 
reason krunner uses multiple threads in the first place, and is in addition 
wildly over-complicated.

If you really don't like a mutex in this class, I can instead stick it into the 
krunner function directly. Thus the thing you don't like would be restricted to 
one obscure corner of plasma, rather than in kdelibs.


- Simeon


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


On Sept. 9, 2012, 10:06 p.m., Simeon Bird wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106242/
> ---
> 
> (Updated Sept. 9, 2012, 10:06 p.m.)
> 
> 
> Review request for kdelibs and Plasma.
> 
> 
> Description
> ---
> 
> Krunner's spellcheck plugin has been pretty broken since 
> bd291d21f096a714a171e7af3a534ba345ca5659 (about two years ago) because it 
> called Sonnet::Speller::setLanguage every time the spellchecker was invoked, 
> which was (very much) not thread-safe.
> 
> This patch makes Sonnet::Speller::setLanguage threadsafe by protecting all 
> access to the internal dict pointer using QReadWriteLock. 
> 
> A related review request is 106244, which adds more fixes to the spellcheck 
> feature.
> 
> 
> This addresses bugs 264779 and 303831.
> http://bugs.kde.org/show_bug.cgi?id=264779
> http://bugs.kde.org/show_bug.cgi?id=303831
> 
> 
> Diffs
> -
> 
>   kdecore/sonnet/speller.cpp b19e74d 
> 
> Diff: http://git.reviewboard.kde.org/r/106242/diff/
> 
> 
> Testing
> ---
> 
> Compiled, installed, used for a week or so, spellchecked a bunch of things.
> 
> 
> Thanks,
> 
> Simeon Bird
> 
>

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

Re: Review Request: Make Sonnet::Speller::setLanguage threadsafe, fixing segfaults in krunner

2012-09-19 Thread Simeon Bird 


> On Sept. 11, 2012, 7:39 p.m., Zack Rusin wrote:
> > That's pretty bonkers. This class was never meant to be used like that. 
> > Holding an object in two threads is not going to work unless you make the 
> > entire communication synchronous effectively reducing all the 
> > multi-threading aspects to a "very complicated single thread". It just 
> > complicates this class.
> > The proper fix is to, instead of trying to synchronize one object owned by 
> > multiple threads, make the speller property of just the thread that does 
> > the spelling (or even just moveToThread it if you have to) and instead of 
> > calling setLanguage from another thread create a signal/slot combination 
> > between the parent thread and the speller thread and send a language change 
> > request by emitting a signal from the parent thread.
> 
> Simeon Bird wrote:
> I understand that this was not in the original design for the class; if 
> it were, the patch would not be necessary. However, the architecture of 
> krunner renders the scheme you describe extremely difficult and messy to 
> implement. 
> This is because krunner posts each set of input to a different thread, 
> and the input contains both the word to be spelt and the language. 
> 
> So, yes, we do actually need to be spelling multiple things in different 
> threads. Which means we have to be careful to not change the language from 
> under them. What you are suggesting won't work because all threads need to do 
> the spelling. I can create an extra thread that does it, and then have it run 
> an event loop fed from the helper threads, but that completely obviates the 
> reason krunner uses multiple threads in the first place, and is in addition 
> wildly over-complicated.
> 
> If you really don't like a mutex in this class, I can instead stick it 
> into the krunner function directly. Thus the thing you don't like would be 
> restricted to one obscure corner of plasma, rather than in kdelibs.
> 
> Zack Rusin wrote:
> No, what I'm saying is that none of the objects in this framework was 
> designed to be used by multiple threads. It's not a matter of missing 
> functionality, you're simply using it wrong. Sonnet shouldn't have to be 
> solving problems of thread syncing in krunner. I don't really know what "not 
> to change the language from under them" means, neither do I understand why 
> connecting a change language signal from some master thread to multiple spell 
> checking threads wouldn't work, but the solution to the problem is not try to 
> shoehorn a paradigm for which a framework was never intended, but to fix the 
> application to use the framework properly. In the worst case you can always 
> create your MTSpeller class in your plugin, use sonner::speller inside it and 
> add whatever locking your heart desires to it.

The trouble with connecting a change language signal from a master thread is 
that there isn't really a master thread. 
There are several threads, each of which may in general be spell-checking 
different languages. 
The most correct and elegant thing, really, is to construct a new speller 
object in each thread 
and use that, but that would be a bit slow, I think, since a new thread is 
spawned on every key-press. 

In any case, thanks for your review - I will try the new-object-creation thing, 
and if slow I will do as you suggest and move the locking to the krunner class. 


- Simeon


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


On Sept. 9, 2012, 10:06 p.m., Simeon Bird wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106242/
> ---
> 
> (Updated Sept. 9, 2012, 10:06 p.m.)
> 
> 
> Review request for kdelibs and Plasma.
> 
> 
> Description
> ---
> 
> Krunner's spellcheck plugin has been pretty broken since 
> bd291d21f096a714a171e7af3a534ba345ca5659 (about two years ago) because it 
> called Sonnet::Speller::setLanguage every time the spellchecker was invoked, 
> which was (very much) not thread-safe.
> 
> This patch makes Sonnet::Speller::setLanguage threadsafe by protecting all 
> access to the internal dict pointer using QReadWriteLock. 
> 
> A related review request is 106244, which adds more fixes to the spellcheck 
> feature.
> 
> 
> This addresses bugs 264779 and 303831.
> http://bugs.kde.org/show_bug.cgi?id=264779
> http://bugs.kde.org/show_bug.cgi?id=303831
> 
> 
> Diffs
> -
> 
>   kdecore/sonnet/speller.cpp b19e74d 
> 
> Diff: http://git.reviewboard.kde.org/r/106242/diff/
> 
> 
> Testing
> ---
> 
> Compiled, installed, used for a week or so, spellchecked a bunch of things.
> 
> 
> Thanks,
> 
> Simeon Bird
> 
>

Re: Review Request: Make Sonnet::Speller::setLanguage threadsafe, fixing segfaults in krunner

2012-09-19 Thread Thomas Lübking 


> On Sept. 11, 2012, 7:39 p.m., Zack Rusin wrote:
> > That's pretty bonkers. This class was never meant to be used like that. 
> > Holding an object in two threads is not going to work unless you make the 
> > entire communication synchronous effectively reducing all the 
> > multi-threading aspects to a "very complicated single thread". It just 
> > complicates this class.
> > The proper fix is to, instead of trying to synchronize one object owned by 
> > multiple threads, make the speller property of just the thread that does 
> > the spelling (or even just moveToThread it if you have to) and instead of 
> > calling setLanguage from another thread create a signal/slot combination 
> > between the parent thread and the speller thread and send a language change 
> > request by emitting a signal from the parent thread.
> 
> Simeon Bird wrote:
> I understand that this was not in the original design for the class; if 
> it were, the patch would not be necessary. However, the architecture of 
> krunner renders the scheme you describe extremely difficult and messy to 
> implement. 
> This is because krunner posts each set of input to a different thread, 
> and the input contains both the word to be spelt and the language. 
> 
> So, yes, we do actually need to be spelling multiple things in different 
> threads. Which means we have to be careful to not change the language from 
> under them. What you are suggesting won't work because all threads need to do 
> the spelling. I can create an extra thread that does it, and then have it run 
> an event loop fed from the helper threads, but that completely obviates the 
> reason krunner uses multiple threads in the first place, and is in addition 
> wildly over-complicated.
> 
> If you really don't like a mutex in this class, I can instead stick it 
> into the krunner function directly. Thus the thing you don't like would be 
> restricted to one obscure corner of plasma, rather than in kdelibs.
> 
> Zack Rusin wrote:
> No, what I'm saying is that none of the objects in this framework was 
> designed to be used by multiple threads. It's not a matter of missing 
> functionality, you're simply using it wrong. Sonnet shouldn't have to be 
> solving problems of thread syncing in krunner. I don't really know what "not 
> to change the language from under them" means, neither do I understand why 
> connecting a change language signal from some master thread to multiple spell 
> checking threads wouldn't work, but the solution to the problem is not try to 
> shoehorn a paradigm for which a framework was never intended, but to fix the 
> application to use the framework properly. In the worst case you can always 
> create your MTSpeller class in your plugin, use sonner::speller inside it and 
> add whatever locking your heart desires to it.
> 
> Simeon Bird wrote:
> The trouble with connecting a change language signal from a master thread 
> is that there isn't really a master thread. 
> There are several threads, each of which may in general be spell-checking 
> different languages. 
> The most correct and elegant thing, really, is to construct a new speller 
> object in each thread 
> and use that, but that would be a bit slow, I think, since a new thread 
> is spawned on every key-press. 
> 
> In any case, thanks for your review - I will try the new-object-creation 
> thing, and if slow I will do as you suggest and move the locking to the 
> krunner class.

> there isn't really a master thread
how is the language changed? gui? -> master thread
block there until all running threads exited, update the lang, continue, ..., 
profit?
(you only have one lineedit, so i assume you effectively only check one lang at 
a time?)


- Thomas


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


On Sept. 9, 2012, 10:06 p.m., Simeon Bird wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106242/
> ---
> 
> (Updated Sept. 9, 2012, 10:06 p.m.)
> 
> 
> Review request for kdelibs and Plasma.
> 
> 
> Description
> ---
> 
> Krunner's spellcheck plugin has been pretty broken since 
> bd291d21f096a714a171e7af3a534ba345ca5659 (about two years ago) because it 
> called Sonnet::Speller::setLanguage every time the spellchecker was invoked, 
> which was (very much) not thread-safe.
> 
> This patch makes Sonnet::Speller::setLanguage threadsafe by protecting all 
> access to the internal dict pointer using QReadWriteLock. 
> 
> A related review request is 106244, which adds more fixes to the spellcheck 
> feature.
> 
> 
> This addresses bugs 264779 and 303831.
> http://bugs.kde.org/show_bug.cg

Re: Review Request: Make KDM compilation optional

2012-09-19 Thread Ralf Jung 


> On Sept. 15, 2012, 9:23 a.m., Martin Gräßlin wrote:
> > given that everything else uses the optional macro that sounds like a valid 
> > improvement to me

Great - if nobody objects, I'll push this tomorrow evening.


- Ralf


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


On Sept. 14, 2012, 10:26 a.m., Ralf Jung wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106444/
> ---
> 
> (Updated Sept. 14, 2012, 10:26 a.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Description
> ---
> 
> This makes compilation of KDM optional. KDE works fine without KDM using 
> another display manager. and considering that even Plasma is optional, I see 
> no reason for KDM to be mandatory.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 30eb084 
> 
> Diff: http://git.reviewboard.kde.org/r/106444/diff/
> 
> 
> Testing
> ---
> 
> Compiled both with default configuration and with BUILD_kdm=Off, verified 
> that login worked and that the kcm is indeed gone.
> 
> 
> Thanks,
> 
> Ralf Jung
> 
>

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

Re: Review Request: Fix setting the size of the calendar of clocks

2012-09-19 Thread Sebastian Kügler 


> On Sept. 18, 2012, 10:13 p.m., Sebastian Kügler wrote:
> > Patch looks good now, thanks a lot!
> > 
> > Please commit it to KDE/4.9 and master.
> 
> Sebastian Kügler wrote:
> Ah, crap. I missed your question about the possible BIC issue. Can 
> someone answer this question?
> 
> """
> Is it okay to remove a no longer needed overload of a virtual function 
> from a class in kde-workspace/libs, or is there some BC constraint?
> """
> (the patch changes updatePreferredSize() to updateSize())
> 
> Please hold off pushing the patch for now, we really don't want to ship 
> any BIC issues into a stable branch. :/
> 
> Marco Martin wrote:
> yes, that patch breaks bc.
> however a library in kde-workspace has no promise of bc.
> only one thing, shouldn't break released software, so it's fine to commit 
> to master but not backporting to 4.9
> 
> Ralf Jung wrote:
> Actually, the patch presented here does not break BC. This function I 
> renamed is part of CalendarPrivate and therefore can not be used from the 
> outside.
> The BC breakage would (possibly) be introduced by removing 
> Calendar::resizeEvent, which is why I just stubbed it out to call the 
> superclass function.

As Marco said, please push only to master. :)

Thanks...


- Sebastian


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


On Sept. 18, 2012, 5:40 p.m., Ralf Jung wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106441/
> ---
> 
> (Updated Sept. 18, 2012, 5:40 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Description
> ---
> 
> Currently, the size of the calendar opened when clicking a clock is sometimes 
> set incorrectly, see the bug I linked to this review request. This patch 
> fixes the bug by setting a proper minimal size on the calendar widget and 
> updating the displayed events after configuration changes.
> 
> 
> This addresses bug 306762.
> http://bugs.kde.org/show_bug.cgi?id=306762
> 
> 
> Diffs
> -
> 
>   libs/plasmaclock/calendar.cpp 7ea70c2 
> 
> Diff: http://git.reviewboard.kde.org/r/106441/diff/
> 
> 
> Testing
> ---
> 
> I verified that the bug is fixed - everything behaved as expected.
> 
> 
> Thanks,
> 
> Ralf Jung
> 
>

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

Re: Review Request: Make Sonnet::Speller::setLanguage threadsafe, fixing segfaults in krunner

2012-09-19 Thread Zack Rusin 


> On Sept. 11, 2012, 7:39 p.m., Zack Rusin wrote:
> > That's pretty bonkers. This class was never meant to be used like that. 
> > Holding an object in two threads is not going to work unless you make the 
> > entire communication synchronous effectively reducing all the 
> > multi-threading aspects to a "very complicated single thread". It just 
> > complicates this class.
> > The proper fix is to, instead of trying to synchronize one object owned by 
> > multiple threads, make the speller property of just the thread that does 
> > the spelling (or even just moveToThread it if you have to) and instead of 
> > calling setLanguage from another thread create a signal/slot combination 
> > between the parent thread and the speller thread and send a language change 
> > request by emitting a signal from the parent thread.
> 
> Simeon Bird wrote:
> I understand that this was not in the original design for the class; if 
> it were, the patch would not be necessary. However, the architecture of 
> krunner renders the scheme you describe extremely difficult and messy to 
> implement. 
> This is because krunner posts each set of input to a different thread, 
> and the input contains both the word to be spelt and the language. 
> 
> So, yes, we do actually need to be spelling multiple things in different 
> threads. Which means we have to be careful to not change the language from 
> under them. What you are suggesting won't work because all threads need to do 
> the spelling. I can create an extra thread that does it, and then have it run 
> an event loop fed from the helper threads, but that completely obviates the 
> reason krunner uses multiple threads in the first place, and is in addition 
> wildly over-complicated.
> 
> If you really don't like a mutex in this class, I can instead stick it 
> into the krunner function directly. Thus the thing you don't like would be 
> restricted to one obscure corner of plasma, rather than in kdelibs.
> 
> Zack Rusin wrote:
> No, what I'm saying is that none of the objects in this framework was 
> designed to be used by multiple threads. It's not a matter of missing 
> functionality, you're simply using it wrong. Sonnet shouldn't have to be 
> solving problems of thread syncing in krunner. I don't really know what "not 
> to change the language from under them" means, neither do I understand why 
> connecting a change language signal from some master thread to multiple spell 
> checking threads wouldn't work, but the solution to the problem is not try to 
> shoehorn a paradigm for which a framework was never intended, but to fix the 
> application to use the framework properly. In the worst case you can always 
> create your MTSpeller class in your plugin, use sonner::speller inside it and 
> add whatever locking your heart desires to it.
> 
> Simeon Bird wrote:
> The trouble with connecting a change language signal from a master thread 
> is that there isn't really a master thread. 
> There are several threads, each of which may in general be spell-checking 
> different languages. 
> The most correct and elegant thing, really, is to construct a new speller 
> object in each thread 
> and use that, but that would be a bit slow, I think, since a new thread 
> is spawned on every key-press. 
> 
> In any case, thanks for your review - I will try the new-object-creation 
> thing, and if slow I will do as you suggest and move the locking to the 
> krunner class.
> 
> Thomas Lübking wrote:
> > there isn't really a master thread
> how is the language changed? gui? -> master thread
> block there until all running threads exited, update the lang, continue, 
> ..., profit?
> (you only have one lineedit, so i assume you effectively only check one 
> lang at a time?)
> 
> Simeon Bird wrote:
> Ah, no, not as simple as that. You're thinking (I think) of a standard 
> spellchecker widget, which has a button marked "language: deutsch", that you 
> click to change the language. 
> 
> However, here we are a krunner plugin, and krunner runs the gui thread. 
> We do not have access to it, or to the dispatcher mechanism, we just have to 
> take the string we are handed, parse it for a language name, and do what we 
> can. So although in 'reality' we only check one lang at a time, krunner 
> pretends to us that we are checking more, and we have to deal with that.

That sounds perfect then, it already handles the signal for you. That just 
means that each spell checking thread gets it own speller and each calls 
setLanguage whenever it's ready. 


- Zack


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


On Sept. 9, 2012, 10:06 p.m., Simeon Bird wrote:
> 
> ---
> This is an autom