Re: Review Request 126087: Move the i18n context from KDeclarative

[Marco Martin]

> On Nov. 17, 2015, 10:05 a.m., Marco Martin wrote:
> > a public symbol could even be spared (and in the process made possibleto 
> > use it from pure qml) by making an import instead that does the 
> > setContextObject() injection in its setupEngine (that would happen 
> > immediately after an import org.kde.i18n statement)
> > 
> > the problem tough would be that then could be hard maintaining 
> > retrocompatibility in kdeclarative (tough an explicit 
> > QQmlEngine::importPlugin *may* work)
> 
> Aleix Pol Gonzalez wrote:
> What if the application already has a ContextObject?
> 
> In fact, then we wouldn't be able to port KDeclarative, AFAIU: 
> https://git.reviewboard.kde.org/r/126088/

that's why it should probably be available also as a singleton..


- Marco


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


On Nov. 18, 2015, 12:26 p.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126087/
> ---
> 
> (Updated Nov. 18, 2015, 12:26 p.m.)
> 
> 
> Review request for KDE Frameworks, Chusslove Illich and Marco Martin.
> 
> 
> Repository: ki18n
> 
> 
> Description
> ---
> 
> The only way to use `i18n*()` so far in QML is by depending on all 
> KDeclarative. This patch moves the necessary bits there so it can be adopted 
> by an application or framework just by offering the needed bits within KI18n.
> This is done by offering an object with methods that map to the `i18n*()` C++ 
> counter-parts.
> 
> 
> Diffs
> -
> 
>   docs/programmers-guide.md 13a5f9d 
>   src/CMakeLists.txt 818595e 
>   src/klocalizedcontext.h PRE-CREATION 
>   src/klocalizedcontext.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/126087/diff/
> 
> 
> Testing
> ---
> 
> Ported KDeclarative, everything still seems to work.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125705: Use LANG for month names in calendar applet

[Martin Klapetek]

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

(Updated Nov. 18, 2015, 5:38 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and Plasma.


Changes
---

Submitted with commit 6f3fed77d59227a9b3ec85d343ffb2443086f7fa by Martin 
Klapetek to branch master.


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


Repository: plasma-framework


Description
---

Simple QDate::longMonthName won't do the job as it
will return the month name using LC_DATE locale which is used
for date formatting etc. So for example, in en_US locale
and cs_CZ LC_DATE, it would return Czech month names while
it should return English ones. So here we force the LANG
locale and take the month name from that.


Diffs
-

  src/declarativeimports/calendar/calendar.cpp 67a08e5 

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


Testing
---

LC_DATE=cs_CZ
LANG=en_US

Month names in calendar applet are now english.


Thanks,

Martin Klapetek

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126078: [OS X] modernising the KIdleTime plugin (WIP!)

[Luca Beltrame]

Il Wed, 18 Nov 2015 17:28:27 +0100, René J.V. Bertin ha scritto:

> If we're starting to call names arguing indeed becomes pointless as the
> apparent lack of actual reading my arguments already suggested. It

In the interest of the CoC and before people heat up, Martin said:

"Thus the code is idiotic"

so I don't see anything like name calling. Anyway, since this is likely 
off-topic, if you feel you've been treated unfairly from a non-development 
point of view, feel free to contact KDE's community working group.

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126087: Move the i18n context from KDeclarative

[Marco Martin]

> On Nov. 17, 2015, 9:58 a.m., Marco Martin wrote:
> > Here's the reason I don't like it:
> > In origin KDeclarative was intended to be a library that only depended from 
> > Qt stuff, to be at most tier 2 (and imports there were supposed to be 
> > qt-only as well) and frameworks applications using QML were supposed to do 
> > it trough KDeclarative..
> > This just highlights a problem happened afterwards, when no frameworks 
> > maintainer wanted any qml binding it the framework's repository, the 
> > kdeclarative repo ended up being pretty much a dumpster where all the 
> > binding of all the frameworks went, making it depend from pretty much 
> > everything.
> > This also means that whoever will need the binding of $framework, it will 
> > probably rather rewrite them, since using kdeclarative means pretty much 
> > depending from all of them.
> > So, fine for making the i18n context independent, but iff the whole 
> > situation gets resolved and each single import that depends from more than 
> > Qt stuff goes either in its framework repository or gets its own.
> 
> Martin Gräßlin wrote:
> I agree with Marco: we should try to split this up again. Somehow the 
> framework starts to remind me of kdelibs4 and in applications I maintain it 
> takes a lot of strength to buy into using it. (Why would I want KIO in KWin?)
> 
> Aleix Pol Gonzalez wrote:
> Isn't it what this patch is about?
> 
> To me it's clear that KIconProvider could go to KIconThemes and 
> KIOAccessManagerFactory could go to KIO.
> 
> Marco Martin wrote:
> we could push that right at the start of the 5.18 cycle so that's done 
> well in advance
> 
> Marco Martin wrote:
> yes, and most of the linking luckily is private. the only thing that is 
> public and a bit out of tune unfortunately is configpropertymap
> 
> Marco Martin wrote:
> in general i'm in favor, i would like to try to make it an import instead 
> tough (the qobject inserted as context object if a custom one wasn't set 
> already and made a QML singleton as well, so that can get accessed by 
> KI18n.i18n("") and such).
> it should work if it can be injected somehow from KDeclarative c++
> 
> Aleix Pol Gonzalez wrote:
> If we do it in a different way (e.g. a singleton), we'll have to get the 
> code ported. That's not backwards-compatible.

to stay compatible, kdeclarative could try to load the plugin on the engine, 
and should be the same as directly setting the context object


- Marco


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


On Nov. 18, 2015, 12:26 p.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126087/
> ---
> 
> (Updated Nov. 18, 2015, 12:26 p.m.)
> 
> 
> Review request for KDE Frameworks, Chusslove Illich and Marco Martin.
> 
> 
> Repository: ki18n
> 
> 
> Description
> ---
> 
> The only way to use `i18n*()` so far in QML is by depending on all 
> KDeclarative. This patch moves the necessary bits there so it can be adopted 
> by an application or framework just by offering the needed bits within KI18n.
> This is done by offering an object with methods that map to the `i18n*()` C++ 
> counter-parts.
> 
> 
> Diffs
> -
> 
>   docs/programmers-guide.md 13a5f9d 
>   src/CMakeLists.txt 818595e 
>   src/klocalizedcontext.h PRE-CREATION 
>   src/klocalizedcontext.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/126087/diff/
> 
> 
> Testing
> ---
> 
> Ported KDeclarative, everything still seems to work.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125619: Refactor KNewPasswordDialog class

[Elvis Angelaccio]

> On Nov. 16, 2015, 12:56 p.m., Heiko Tietze wrote:
> > Ship It!

I'm assuming that yours is a ship-it only from the usability side, right?


- Elvis


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


On Oct. 13, 2015, 1:33 p.m., Elvis Angelaccio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125619/
> ---
> 
> (Updated Oct. 13, 2015, 1:33 p.m.)
> 
> 
> Review request for KDE Frameworks, KDE Usability, Christoph Feck, and David 
> Faure.
> 
> 
> Repository: kwidgetsaddons
> 
> 
> Description
> ---
> 
> Follow-up of the KNewPasswordWidget review.
> By introducing a KNewPasswordWidget in the .ui file, we can:
> 
> - remove the code that has been moved to KNewPasswordWidget
> - enable the KNewPasswordWidget additional features
> 
> The only feature that is currently not available is the ability to remove the 
> strength bar. Do we want to allow developers to show a KNewPasswordDialog 
> without a strenght bar?
> 
> 
> Diffs
> -
> 
>   src/knewpassworddialog.h 1ac7b2151f1e88681a15ce21465449cedb871f1e 
>   src/knewpassworddialog.cpp bd7459acf3c72bc6dc0711da6086213d5111d5c3 
>   src/knewpassworddialog.ui 55a1f62cf4ba6d6c87b2c47742ba3bcd531ebfe8 
> 
> Diff: https://git.reviewboard.kde.org/r/125619/diff/
> 
> 
> Testing
> ---
> 
> Trying to insert a password which is too short or too weak or empty, works as 
> expected.
> 
> 
> File Attachments
> 
> 
> knewpassworddialog1.png
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2015/10/13/62bf21f6-1dcf-46c3-8b1d-146500b5f629__knewpassworddialog1.png
> knewpassworddialog2.png
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2015/10/13/b2e2666f-ce19-4df3-b6ba-25c13fc370ae__knewpassworddialog2.png
> knewpassworddialog3.png
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2015/10/13/b9536088-0cc4-470b-a211-6db4ca79aa2f__knewpassworddialog3.png
> knewpassworddialog4.png
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2015/10/13/4f826716-9ed2-4ecb-8cbe-4fb50abf1086__knewpassworddialog4.png
> 
> 
> Thanks,
> 
> Elvis Angelaccio
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126078: [OS X] modernising the KIdleTime plugin (WIP!)

[René J . V . Bertin]

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

(Updated Nov. 18, 2015, 5:35 p.m.)


Status
--

This change has been discarded.


Review request for KDE Software on Mac OS X, KDE Frameworks and Dario Freddi.


Repository: kidletime


Description
---

I noticed that the KIdleTime example doesn't work properly on OS X, and that 
the plugin for OS X still uses the deprecated Carbon-based algorithm that I 
already patched for KDE4.

This patch is a work-in-progress (hence the qDebugs) update to use IOKit, 
IORegistry and CoreServices to do idle-time calculation as it should be done, 
and allow simulated user activity through a "less deprecated" function.


Diffs
-

  src/plugins/osx/CMakeLists.txt e1b50b8 

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


Testing
---

On OS X 10.9 with Qt 5.5.1 and frameworks 5.16.0 .

The example now works: when I set a QTimer with interval==0, the expected wait 
for user input (`resumingFromIdle` signal) works. However, I am getting a 
`stopCatchingIdleEvents` signal which means the application waits forever, 
without ever getting to compare idle time to the list of timeouts.
I haven't been able to figure out where that signal comes from, nor why this 
doesn't happen on Linux.

Surely I'm missing something, but what?


Thanks,

René J.V. Bertin

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126078: [OS X] modernising the KIdleTime plugin (WIP!)

[René J . V . Bertin]

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

(Updated Nov. 18, 2015, 5:35 p.m.)


Review request for KDE Software on Mac OS X, KDE Frameworks and Dario Freddi.


Changes
---

AFAIC you can just remove the whole OS X plugin, I'm not going to commit 
something I don't stand behind.


Repository: kidletime


Description
---

I noticed that the KIdleTime example doesn't work properly on OS X, and that 
the plugin for OS X still uses the deprecated Carbon-based algorithm that I 
already patched for KDE4.

This patch is a work-in-progress (hence the qDebugs) update to use IOKit, 
IORegistry and CoreServices to do idle-time calculation as it should be done, 
and allow simulated user activity through a "less deprecated" function.


Diffs (updated)
-

  src/plugins/osx/CMakeLists.txt e1b50b8 

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


Testing
---

On OS X 10.9 with Qt 5.5.1 and frameworks 5.16.0 .

The example now works: when I set a QTimer with interval==0, the expected wait 
for user input (`resumingFromIdle` signal) works. However, I am getting a 
`stopCatchingIdleEvents` signal which means the application waits forever, 
without ever getting to compare idle time to the list of timeouts.
I haven't been able to figure out where that signal comes from, nor why this 
doesn't happen on Linux.

Surely I'm missing something, but what?


Thanks,

René J.V. Bertin

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125960: Fix build of some projects using ecm_create_qm_loader()

[Aleix Pol Gonzalez]

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

(Updated Nov. 18, 2015, 4:11 p.m.)


Status
--

This change has been discarded.


Review request for Extra Cmake Modules and KDE Frameworks.


Repository: extra-cmake-modules


Description
---

Makes sure the variable is properly initialized.


Diffs
-

  modules/ECMPoQmTools.cmake 22258dc 

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


Testing
---

This error won't happen anymore:
https://build.kde.org/job/analitza%20master%20kf5-qt5/12/PLATFORM=Linux,compiler=gcc/console


Thanks,

Aleix Pol Gonzalez

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126078: [OS X] modernising the KIdleTime plugin (WIP!)

[René J . V . Bertin]

On Wednesday November 18 2015 15:01:59 Martin Gräßlin wrote:

> idiotic

If we're starting to call names arguing indeed becomes pointless as the 
apparent lack of actual reading my arguments already suggested. It leaves me 
with no other choice but to discard the RR and put up a mirror in place.

R
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126105: Fix yet another crash in Dolphin when Baloo isn't running

[Thomas Lübking]

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


Errr... you intend to crash applications depending on whether some file is 
present??
Please fix the actual bug instead of such workaround, got a backtrace?

- Thomas Lübking


On Nov. 18, 2015, 7:40 nachm., Boudhayan Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126105/
> ---
> 
> (Updated Nov. 18, 2015, 7:40 nachm.)
> 
> 
> Review request for Baloo, KDE Frameworks, Pinak Ahuja, and Vishesh Handa.
> 
> 
> Repository: baloo
> 
> 
> Description
> ---
> 
> Add a check in Baloo::Database::open() to return false if we're opening the 
> database in ReadOnly mode and the database doesn't exist. This fixes a crash 
> in Dolphin when Baloo isn't running.
> 
> This isn't the entire fix - one will also have to remove 
> ~/.local/share/baloo/index to not crash anymore; this patch prevents the file 
> from being recreated everytime Baloo::Database::open() is run, and re-causing 
> the crash.
> 
> 
> Diffs
> -
> 
>   src/engine/database.cpp 4f0579f 
> 
> Diff: https://git.reviewboard.kde.org/r/126105/diff/
> 
> 
> Testing
> ---
> 
> Builds, runs, doesn't crash anymore, the works.
> 
> 
> Thanks,
> 
> Boudhayan Gupta
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 126105: Fix yet another crash in Dolphin when Baloo isn't running

[Boudhayan Gupta]

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

Review request for Baloo, KDE Frameworks, Pinak Ahuja, and Vishesh Handa.


Repository: baloo


Description
---

Add a check in Baloo::Database::open() to return false if we're opening the 
database in ReadOnly mode and the database doesn't exist. This fixes a crash in 
Dolphin when Baloo isn't running.

This isn't the entire fix - one will also have to remove 
~/.local/share/baloo/index to not crash anymore; this patch prevents the file 
from being recreated everytime Baloo::Database::open() is run, and re-causing 
the crash.


Diffs
-

  src/engine/database.cpp 4f0579f 

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


Testing
---

Builds, runs, doesn't crash anymore, the works.


Thanks,

Boudhayan Gupta

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126105: Fix yet another crash in Dolphin when Baloo isn't running

[Boudhayan Gupta]

> On Nov. 19, 2015, 1:36 a.m., Thomas Lübking wrote:
> > Errr... you intend to crash applications depending on whether some file is 
> > present??
> > Please fix the actual bug instead of such workaround, got a backtrace?

Err... it **doesn't** cause a crash with the patch, causes a crash without it. 
This is a perfectly valid fix - this function is **supposed** to return false 
if the database could not be opened, for any reason whatsoever.


- Boudhayan


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


On Nov. 19, 2015, 1:10 a.m., Boudhayan Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126105/
> ---
> 
> (Updated Nov. 19, 2015, 1:10 a.m.)
> 
> 
> Review request for Baloo, KDE Frameworks, Pinak Ahuja, and Vishesh Handa.
> 
> 
> Repository: baloo
> 
> 
> Description
> ---
> 
> Add a check in Baloo::Database::open() to return false if we're opening the 
> database in ReadOnly mode and the database doesn't exist. This fixes a crash 
> in Dolphin when Baloo isn't running.
> 
> This isn't the entire fix - one will also have to remove 
> ~/.local/share/baloo/index to not crash anymore; this patch prevents the file 
> from being recreated everytime Baloo::Database::open() is run, and re-causing 
> the crash.
> 
> 
> Diffs
> -
> 
>   src/engine/database.cpp 4f0579f 
> 
> Diff: https://git.reviewboard.kde.org/r/126105/diff/
> 
> 
> Testing
> ---
> 
> Builds, runs, doesn't crash anymore, the works.
> 
> 
> Thanks,
> 
> Boudhayan Gupta
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126105: Fix yet another crash in Dolphin when Baloo isn't running

[Thomas Lübking]

> On Nov. 18, 2015, 8:06 nachm., Thomas Lübking wrote:
> > Errr... you intend to crash applications depending on whether some file is 
> > present??
> > Please fix the actual bug instead of such workaround, got a backtrace?
> 
> Boudhayan Gupta wrote:
> Err... it **doesn't** cause a crash with the patch, causes a crash 
> without it. This is a perfectly valid fix - this function is **supposed** to 
> return false if the database could not be opened, for any reason whatsoever.

I didn't say this patch would introduce a crash.
I said that "something" crashes for "some" reason and you work around that by 
testing whether a file is present.
Your own commit message clearly says that if - for any reason - 
~/.local/share/baloo/index exists (eg. i touch it), things will crash again, 
what means that the actual problem is not resolved but just shadowed.


- Thomas


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


On Nov. 18, 2015, 7:40 nachm., Boudhayan Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126105/
> ---
> 
> (Updated Nov. 18, 2015, 7:40 nachm.)
> 
> 
> Review request for Baloo, KDE Frameworks, Pinak Ahuja, and Vishesh Handa.
> 
> 
> Repository: baloo
> 
> 
> Description
> ---
> 
> Add a check in Baloo::Database::open() to return false if we're opening the 
> database in ReadOnly mode and the database doesn't exist. This fixes a crash 
> in Dolphin when Baloo isn't running.
> 
> This isn't the entire fix - one will also have to remove 
> ~/.local/share/baloo/index to not crash anymore; this patch prevents the file 
> from being recreated everytime Baloo::Database::open() is run, and re-causing 
> the crash.
> 
> 
> Diffs
> -
> 
>   src/engine/database.cpp 4f0579f 
> 
> Diff: https://git.reviewboard.kde.org/r/126105/diff/
> 
> 
> Testing
> ---
> 
> Builds, runs, doesn't crash anymore, the works.
> 
> 
> Thanks,
> 
> Boudhayan Gupta
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126105: Fix yet another crash in Dolphin when Baloo isn't running

[Boudhayan Gupta]

> On Nov. 19, 2015, 1:36 a.m., Thomas Lübking wrote:
> > Errr... you intend to crash applications depending on whether some file is 
> > present??
> > Please fix the actual bug instead of such workaround, got a backtrace?
> 
> Boudhayan Gupta wrote:
> Err... it **doesn't** cause a crash with the patch, causes a crash 
> without it. This is a perfectly valid fix - this function is **supposed** to 
> return false if the database could not be opened, for any reason whatsoever.
> 
> Thomas Lübking wrote:
> I didn't say this patch would introduce a crash.
> I said that "something" crashes for "some" reason and you work around 
> that by testing whether a file is present.
> Your own commit message clearly says that if - for any reason - 
> ~/.local/share/baloo/index exists (eg. i touch it), things will crash again, 
> what means that the actual problem is not resolved but just shadowed.

Why would you want to touch ~/.local/share/baloo/index, if you disable baloo?

Let me explain - balooctl disable works by disabling indexing and removing 
~/.local/share/baloo/index. If baloo is disabled, that file is not supposed to 
exist. A couple of months ago we were even trying to figure out how that file 
came to exist on systems with Baloo disabled and now I've found out. So this 
fixes that too.

Now the real problem. Before this patch, Baloo::Database::open() would just 
create an empty database. Fair enough, except that db->open() would return true 
in places where you clearly have an invalid database. This would still work 
(i.e., not crash), because invalid in this context is empty. However, once you 
select a ton of files (the number is very weird  - 158 or something) LMDB would 
scream at you with this problem - **MDB_READERS_FULL: Environment maxreaders 
limit reached**

There are two ways to solve this - increase the maxreaders limit in LMDB, or 
return false if the database doesn't exist. The first option is when you want 
to go all Linus - *we do not break userspace*, but this way is the more correct 
way of solving this, because again, on systems where Baloo is disabled, the 
index file isn't supposed to exist at all.

I was afraid I was opening up a can of worms by adding yet another condition 
for db->open() to return false, because I was afraid there were places where a 
check on db->open()'s return value was missing (and someone would try to do 
operations on an invalid database). However, I've done all the things that used 
to trigger crashes before and nothing's crashed yet. With Baloo being both 
enabled and disabled.


- Boudhayan


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


On Nov. 19, 2015, 1:10 a.m., Boudhayan Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126105/
> ---
> 
> (Updated Nov. 19, 2015, 1:10 a.m.)
> 
> 
> Review request for Baloo, KDE Frameworks, Pinak Ahuja, and Vishesh Handa.
> 
> 
> Repository: baloo
> 
> 
> Description
> ---
> 
> Add a check in Baloo::Database::open() to return false if we're opening the 
> database in ReadOnly mode and the database doesn't exist. This fixes a crash 
> in Dolphin when Baloo isn't running.
> 
> This isn't the entire fix - one will also have to remove 
> ~/.local/share/baloo/index to not crash anymore; this patch prevents the file 
> from being recreated everytime Baloo::Database::open() is run, and re-causing 
> the crash.
> 
> 
> Diffs
> -
> 
>   src/engine/database.cpp 4f0579f 
> 
> Diff: https://git.reviewboard.kde.org/r/126105/diff/
> 
> 
> Testing
> ---
> 
> Builds, runs, doesn't crash anymore, the works.
> 
> 
> Thanks,
> 
> Boudhayan Gupta
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126078: [OS X] modernising the KIdleTime plugin (WIP!)

[René J . V . Bertin]

> On Nov. 18, 2015, 8:22 a.m., Martin Gräßlin wrote:
> > src/plugins/osx/macpoller.cpp, lines 222-227
> > 
> >
> > seriously? You care about idle timeouts below 5 msec? This is a 
> > framework to tell the application whether the user doesn't use input 
> > devices. I don't know how fast you type, but I'm relatively certain that it 
> > takes more than 5 msec to move my finger from one key to another. What 
> > exactly is that you want to detect here? An event after each key press, 
> > maybe even between a key press and a key release? Because that's probably 
> > already more about 5 msec.

The timer is used only for the detection of idle timeouts (so *absence* of user 
input), and the precision with which it fires determines the accuracy with 
which those events can be timed. This is currently inaccessible anyway, but I 
don't believe in doing things half-bakedly. One might question whether it makes 
sense to allow the framework to work at high precision (I more or less agree 
that remains to be seen). But I don't think it makes sense to do that and then 
leave the QTimer in a mode where the requested precision cannot be met.


- René J.V.


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


On Nov. 17, 2015, 10:13 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126078/
> ---
> 
> (Updated Nov. 17, 2015, 10:13 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X, KDE Frameworks and Dario Freddi.
> 
> 
> Repository: kidletime
> 
> 
> Description
> ---
> 
> I noticed that the KIdleTime example doesn't work properly on OS X, and that 
> the plugin for OS X still uses the deprecated Carbon-based algorithm that I 
> already patched for KDE4.
> 
> This patch is a work-in-progress (hence the qDebugs) update to use IOKit, 
> IORegistry and CoreServices to do idle-time calculation as it should be done, 
> and allow simulated user activity through a "less deprecated" function.
> 
> 
> Diffs
> -
> 
>   src/plugins/osx/CMakeLists.txt e1b50b8 
>   src/plugins/osx/macpoller.h ef51ea5 
>   src/plugins/osx/macpoller.cpp ad9c10f 
>   src/plugins/osx/macpoller_helper.mm PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/126078/diff/
> 
> 
> Testing
> ---
> 
> On OS X 10.9 with Qt 5.5.1 and frameworks 5.16.0 .
> 
> The example now works: when I set a QTimer with interval==0, the expected 
> wait for user input (`resumingFromIdle` signal) works. However, I am getting 
> a `stopCatchingIdleEvents` signal which means the application waits forever, 
> without ever getting to compare idle time to the list of timeouts.
> I haven't been able to figure out where that signal comes from, nor why this 
> doesn't happen on Linux.
> 
> Surely I'm missing something, but what?
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126078: [OS X] modernising the KIdleTime plugin (WIP!)

[René J . V . Bertin]

> On Nov. 17, 2015, 11:58 p.m., Lamarque Souza wrote:
> > src/plugins/osx/macpoller.h, line 24
> > 
> >
> > Nitpick: this should go after #include 

Any guidelines that dictate this?


> On Nov. 17, 2015, 11:58 p.m., Lamarque Souza wrote:
> > src/plugins/osx/macpoller.cpp, line 127
> > 
> >
> > Initialize m_nativeGrabber here too.

Oops, indeed it could be left un-initialised. My bad.


> On Nov. 17, 2015, 11:58 p.m., Lamarque Souza wrote:
> > src/plugins/osx/macpoller.cpp, line 178
> > 
> >
> > Use qCWarning instead.

I presume that should be using KIDLETIME for the category?

Isn't it possible to get the `KIDLETIME()` symbol through libKF5KIdleTime 
rather than having to pull in `../../logging.cpp`?


> On Nov. 17, 2015, 11:58 p.m., Lamarque Souza wrote:
> > src/plugins/osx/macpoller.cpp, line 213
> > 
> >
> > Shouldn't you check the return value of this method?

As far as I can see all it can be used for is to print a warning, right?


> On Nov. 17, 2015, 11:58 p.m., Lamarque Souza wrote:
> > src/plugins/osx/macpoller.cpp, line 215
> > 
> >
> > This should be changed to "return true", right?

Yeah, that seems logical, but I don't see any documentation on what 
setupPoller() should return. Then again the upstream code doesn't appear to use 
the return value anyway so the question is a bit moot ...


> On Nov. 17, 2015, 11:58 p.m., Lamarque Souza wrote:
> > src/plugins/osx/macpoller.cpp, line 300
> > 
> >
> > Remove commented code.

OK, but among the final things to do before committing :)


> On Nov. 17, 2015, 11:58 p.m., Lamarque Souza wrote:
> > src/plugins/osx/macpoller.cpp, line 353
> > 
> >
> > Remove commented code.

The bingo isn't mine!

Oh, the code. As above, in final cleanup :)


> On Nov. 17, 2015, 11:58 p.m., Lamarque Souza wrote:
> > src/plugins/osx/macpoller.cpp, line 404
> > 
> >
> > I do not get the "Unsetting m_catch is enough" comment. What is it 
> > supposed to mean since unsetting m_catch is the original code.

The original code worked rather differently. It used a mechanism with an 
internal poller, but you're right, the wording as it is makes sense only to 
someone who went about replacing that original code and made a number of 
(trial-and-)errors on the way.


> On Nov. 17, 2015, 11:58 p.m., Lamarque Souza wrote:
> > src/plugins/osx/macpoller.cpp, line 422
> > 
> >
> > Remove trailing space and shouldn't the commented code in this method 
> > be removed?

I put the commented code as a reference for alternative approaches that could 
be investigated once more if ever `updateSystemActivity` is removed. If there's 
a policy against such comments I can also store the snippets in a separate 
file, but that means adding one. What's preferable?


> On Nov. 17, 2015, 11:58 p.m., Lamarque Souza wrote:
> > src/plugins/osx/macpoller_helper.mm, line 43
> > 
> >
> > Use qCDebug instead.

That one actually got through and was never meant to!


> On Nov. 17, 2015, 11:58 p.m., Lamarque Souza wrote:
> > src/plugins/osx/macpoller_helper.mm, line 78
> > 
> >
> > There is no point in doing this if the next line will delete 
> > nativeGrabber.

I've learned to be fool-proof with this kind of thing (I don't trust `delete` 
to zero memory before releasing it; there's no `~CocoaEventFilter()`) but what 
does make it redundant here is setting `m_nativeGrabber = 0` just after 
deleting `nativeGrabber`.


- René J.V.


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


On Nov. 17, 2015, 10:13 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126078/
> ---
> 
> (Updated Nov. 17, 2015, 10:13 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X, KDE Frameworks and Dario Freddi.
> 
> 
> Repository: kidletime
> 
> 
> 

Re: KIdleTime: early and/or failing/rejected timeout detection?

[René J . V . Bertin]

Actually, there is one thing I may have missed in the documentation, if it's 
there:

Is KIdleTime supposed to detect system idling, or application idling? As it 
happens, it should be possible to make a distinction between the two with the 
latest implementation I made.

As to using an agent to centralise all polling: I think that agent would either 
have to use poll timer with a sufficiently short interval to serve all clients, 
or use a poll timer per current client. Evidently the latter option cancels any 
benefit of moving to an agent-based architecture (leaving only the IPC 
overhead), while the former will lead to worse overhead than what my current 
implementation allows.
That current implementation is not expensive in its default configuration (and 
that configuration is currently decided at build time), but I think that good 
programming practice would call anyway for deactivating any KIdleTime instances 
when they are not in use, regardless of whether they're based on alarms or on 
polling.

I have no idea how many applications will be using this framework concurrently 
at any given time, but it seems reasonable to expect that there will be less 
than on a system running a KF5 plasma desktop. I see it's used in Baloo, for 
instance, but that framework has dropped support for OS X. 

There's also always the possibility to reconsider implementing an Xcb-style 
alarm based mechanism that relies on a helper process, if it turns out in the 
future that KIdleTime/Mac is causing significant amounts of overhead. But 
KIdleTime has changed only minimally from KDE4 to KF5, and it is certainly not 
the case that it has led to any observable amount of continuous overhead until 
now.

R

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126087: Move the i18n context from KDeclarative

[Aleix Pol Gonzalez]

> On Nov. 17, 2015, 9:43 a.m., Chusslove Illich wrote:
> > Could you also document the usage in docs/programmers-guide.md (section 
> > #link_cat)? I'm not much into QML, so it would help me understand the 
> > implications of the usage.
> > 
> > It seems to me other three series of calls (xi18n*, ki18n*, kxi18n*) should 
> > be made available as well.
> 
> Aleix Pol Gonzalez wrote:
> I don't think we want the `k*i18n*` version of the calls, as they return 
> KLocalizedString and we probably don't need to deal with it from QML.
> 
> Regarding `xi18n*`, I see that we have counter-parts for every of the 
> `i18n*` calls. Could you explain a bit what does it do differently? Is it to 
> support the xml-like markup?
> 
> Chusslove Illich wrote:
> KLocalizedString exposes the lower-level interface, and more possible 
> argument combinations, so in principle k*i18n* can be useful even in 
> one-liners ending with .toString(). Is there any particular reason not to 
> have it?
> 
> Yes, xi18n* are for ki18n's own XML markup. So if someone likes to use 
> xi18n* generally in the whole program, he should want to use it in QML files 
> as well.

I added xi18n*.

Regarding KLocalizedString, I would wait and assess how we want it to be used 
later on. It largely relies on type system overloads and I'm not sure how well 
Qt would be able to bind to it.


- Aleix


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


On Nov. 16, 2015, 3:55 p.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126087/
> ---
> 
> (Updated Nov. 16, 2015, 3:55 p.m.)
> 
> 
> Review request for KDE Frameworks, Chusslove Illich and Marco Martin.
> 
> 
> Repository: ki18n
> 
> 
> Description
> ---
> 
> The only way to use `i18n*()` so far in QML is by depending on all 
> KDeclarative. This patch moves the necessary bits there so it can be adopted 
> by an application or framework just by offering the needed bits within KI18n.
> This is done by offering an object with methods that map to the `i18n*()` C++ 
> counter-parts.
> 
> 
> Diffs
> -
> 
>   src/klocalizedcontext.cpp PRE-CREATION 
>   src/CMakeLists.txt 818595e 
>   src/klocalizedcontext.h PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/126087/diff/
> 
> 
> Testing
> ---
> 
> Ported KDeclarative, everything still seems to work.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125705: Use LANG for month names in calendar applet

[David Edmundson]

> On Oct. 28, 2015, 8:44 p.m., Martin Klapetek wrote:
> > So...any word on this?
> 
> Albert Astals Cid wrote:
> Re-reading your comment, if i understand it correctly, you claim it is a 
> bug in Qt that it does obey LC_DATE for month names when according to you 
> LC_DATE is only for the formatting and should be using LANG, right? 
> Why not fix it upstream then?
> 
> Martin Klapetek wrote:
> I don't claim it's a bug as I simply don't know if it's correct behavior 
> or not, just that the way it works does not suit our intended use case, 
> because we encourage our users to mess with LC_* variables through the 
> Formats KCM, which makes it prone to mixed configurations like LC_DATE=cs_CZ 
> and LANG=en_US.
> 
> But nevertheless a) David Edmundson has a patch in the works so let's see 
> if they think it's a bug and b) we'd need something for the meantime anyway.
> 
> David Edmundson wrote:
> Just uploaded: https://codereview.qt-project.org/139295
> 
> Martin Klapetek wrote:
> Given the Qt patch was rejected, I'll ship this if noone objects.

+1


- David


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


On Oct. 20, 2015, 7:04 p.m., Martin Klapetek wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125705/
> ---
> 
> (Updated Oct. 20, 2015, 7:04 p.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Bugs: 353715
> http://bugs.kde.org/show_bug.cgi?id=353715
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> Simple QDate::longMonthName won't do the job as it
> will return the month name using LC_DATE locale which is used
> for date formatting etc. So for example, in en_US locale
> and cs_CZ LC_DATE, it would return Czech month names while
> it should return English ones. So here we force the LANG
> locale and take the month name from that.
> 
> 
> Diffs
> -
> 
>   src/declarativeimports/calendar/calendar.cpp 67a08e5 
> 
> Diff: https://git.reviewboard.kde.org/r/125705/diff/
> 
> 
> Testing
> ---
> 
> LC_DATE=cs_CZ
> LANG=en_US
> 
> Month names in calendar applet are now english.
> 
> 
> Thanks,
> 
> Martin Klapetek
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126087: Move the i18n context from KDeclarative

[Aleix Pol Gonzalez]

> On Nov. 17, 2015, 11:05 a.m., Marco Martin wrote:
> > a public symbol could even be spared (and in the process made possibleto 
> > use it from pure qml) by making an import instead that does the 
> > setContextObject() injection in its setupEngine (that would happen 
> > immediately after an import org.kde.i18n statement)
> > 
> > the problem tough would be that then could be hard maintaining 
> > retrocompatibility in kdeclarative (tough an explicit 
> > QQmlEngine::importPlugin *may* work)

What if the application already has a ContextObject?

In fact, then we wouldn't be able to port KDeclarative, AFAIU: 
https://git.reviewboard.kde.org/r/126088/


- Aleix


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


On Nov. 18, 2015, 1:26 p.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126087/
> ---
> 
> (Updated Nov. 18, 2015, 1:26 p.m.)
> 
> 
> Review request for KDE Frameworks, Chusslove Illich and Marco Martin.
> 
> 
> Repository: ki18n
> 
> 
> Description
> ---
> 
> The only way to use `i18n*()` so far in QML is by depending on all 
> KDeclarative. This patch moves the necessary bits there so it can be adopted 
> by an application or framework just by offering the needed bits within KI18n.
> This is done by offering an object with methods that map to the `i18n*()` C++ 
> counter-parts.
> 
> 
> Diffs
> -
> 
>   docs/programmers-guide.md 13a5f9d 
>   src/CMakeLists.txt 818595e 
>   src/klocalizedcontext.h PRE-CREATION 
>   src/klocalizedcontext.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/126087/diff/
> 
> 
> Testing
> ---
> 
> Ported KDeclarative, everything still seems to work.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126087: Move the i18n context from KDeclarative

[Aleix Pol Gonzalez]

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

(Updated Nov. 18, 2015, 1:26 p.m.)


Review request for KDE Frameworks, Chusslove Illich and Marco Martin.


Changes
---

Updated with `ki18n*` calls.


Repository: ki18n


Description (updated)
---

The only way to use `i18n*()` so far in QML is by depending on all 
KDeclarative. This patch moves the necessary bits there so it can be adopted by 
an application or framework just by offering the needed bits within KI18n.
This is done by offering an object with methods that map to the `i18n*()` C++ 
counter-parts.


Diffs (updated)
-

  docs/programmers-guide.md 13a5f9d 
  src/CMakeLists.txt 818595e 
  src/klocalizedcontext.h PRE-CREATION 
  src/klocalizedcontext.cpp PRE-CREATION 

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


Testing
---

Ported KDeclarative, everything still seems to work.


Thanks,

Aleix Pol Gonzalez

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126087: Move the i18n context from KDeclarative

[Chusslove Illich]

> On Нов. 17, 2015, 9:43 пре п., Chusslove Illich wrote:
> > Could you also document the usage in docs/programmers-guide.md (section 
> > #link_cat)? I'm not much into QML, so it would help me understand the 
> > implications of the usage.
> > 
> > It seems to me other three series of calls (xi18n*, ki18n*, kxi18n*) should 
> > be made available as well.
> 
> Aleix Pol Gonzalez wrote:
> I don't think we want the `k*i18n*` version of the calls, as they return 
> KLocalizedString and we probably don't need to deal with it from QML.
> 
> Regarding `xi18n*`, I see that we have counter-parts for every of the 
> `i18n*` calls. Could you explain a bit what does it do differently? Is it to 
> support the xml-like markup?

KLocalizedString exposes the lower-level interface, and more possible argument 
combinations, so in principle k*i18n* can be useful even in one-liners ending 
with .toString(). Is there any particular reason not to have it?

Yes, xi18n* are for ki18n's own XML markup. So if someone likes to use xi18n* 
generally in the whole program, he should want to use it in QML files as well.


- Chusslove


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


On Нов. 16, 2015, 3:55 по п., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126087/
> ---
> 
> (Updated Нов. 16, 2015, 3:55 по п.)
> 
> 
> Review request for KDE Frameworks, Chusslove Illich and Marco Martin.
> 
> 
> Repository: ki18n
> 
> 
> Description
> ---
> 
> The only way to use `i18n*()` so far in QML is by depending on all 
> KDeclarative. This patch moves the necessary bits there so it can be adopted 
> by an application or framework just by offering the needed bits within KI18n.
> This is done by offering an object with methods that map to the `i18n*()` C++ 
> counter-parts.
> 
> 
> Diffs
> -
> 
>   src/klocalizedcontext.cpp PRE-CREATION 
>   src/CMakeLists.txt 818595e 
>   src/klocalizedcontext.h PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/126087/diff/
> 
> 
> Testing
> ---
> 
> Ported KDeclarative, everything still seems to work.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: KIdleTime: early and/or failing/rejected timeout detection?

[Martin Gräßlin]

Am 2015-11-18 12:33, schrieb René J. V.  Bertin:
Actually, there is one thing I may have missed in the documentation, if 
it's

there:

Is KIdleTime supposed to detect system idling, or application idling? 
As it
happens, it should be possible to make a distinction between the two 
with the

latest implementation I made.


What's an "idle application"?



As to using an agent to centralise all polling: I think that agent 
would either
have to use poll timer with a sufficiently short interval to serve all 
clients,
or use a poll timer per current client. Evidently the latter option 
cancels any

benefit of moving to an agent-based architecture (leaving only the IPC
overhead), while the former will lead to worse overhead than what my 
current

implementation allows.
That current implementation is not expensive in its default 
configuration (and
that configuration is currently decided at build time), but I think 
that good
programming practice would call anyway for deactivating any KIdleTime 
instances
when they are not in use, regardless of whether they're based on alarms 
or on

polling.


Let's assume: you have two applications performing polling at a 3 sec 
interval: this means two applications are woken up every three seconds. 
That's already 100 % more wakeups compared to a daemon solution.


The number of applications using the agent doesn't affect the usage at 
all. Why should an agent with two clients at 3 sec interval need more 
timeouts?


At max such an agent approach needs to poll in an interval serving the 
shortest timeout. This means it's as good as this one client. The other 
ones come for free. If I have one with a three second timeout and one 
with a five second interval, I only need to poll for the three second 
interval. Only if that one is hit, the five second interval becomes 
interesting.


There's also always the possibility to reconsider implementing an 
Xcb-style
alarm based mechanism that relies on a helper process, if it turns out 
in the
future that KIdleTime/Mac is causing significant amounts of overhead. 
But
KIdleTime has changed only minimally from KDE4 to KF5, and it is 
certainly not
the case that it has led to any observable amount of continuous 
overhead until

now.


NO POLLING! Given your reply here, you did not even properly consider my 
argument at all. Which is sad. I'm spending quite some time here to help 
with the OSX implementation. I could also do a "what do I care about 
OSX?" Instead I spent quite some time on it.


Cheers
Martin
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: KIdleTime: early and/or failing/rejected timeout detection?

[Christoph Cullmann]

Hi,

>> There's also always the possibility to reconsider implementing an
>> Xcb-style
>> alarm based mechanism that relies on a helper process, if it turns out
>> in the
>> future that KIdleTime/Mac is causing significant amounts of overhead.
>> But
>> KIdleTime has changed only minimally from KDE4 to KF5, and it is
>> certainly not
>> the case that it has led to any observable amount of continuous
>> overhead until
>> now.
> 
> NO POLLING! Given your reply here, you did not even properly consider my
> argument at all. Which is sad. I'm spending quite some time here to help
> with the OSX implementation. I could also do a "what do I care about
> OSX?" Instead I spent quite some time on it.
I can only tell: Better no implementation than any polling implementation.

The idea of that framework is to give applications a "efficient" way to schedule
stuff to do if idle (at least I thought so).

Greetings
Christoph

-- 
- Dr.-Ing. Christoph Cullmann -
AbsInt Angewandte Informatik GmbH  Email: cullm...@absint.com
Science Park 1 Tel:   +49-681-38360-22
66123 Saarbrücken  Fax:   +49-681-38360-20
GERMANYWWW:   http://www.AbsInt.com

Geschäftsführung: Dr.-Ing. Christian Ferdinand
Eingetragen im Handelsregister des Amtsgerichts Saarbrücken, HRB 11234
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126078: [OS X] modernising the KIdleTime plugin (WIP!)

[René J . V . Bertin]

> On Nov. 17, 2015, 11:58 p.m., Lamarque Souza wrote:
> > src/plugins/osx/macpoller_helper.mm, line 54
> > 
> >
> > Usually when a new operation returns 0 it is because system is on short 
> > on RAM memory (or memory is too fragmented). I would add assert here 
> > instead of silently ignoring the failure to allocate memory.

And I would argue that it is up to the calling software to decide how to react 
to a failure to set up idle detection: in this case it's only the "resume from 
idle" functionality that can no longer work. The idle timeout detection feature 
should still be able to function (possibly by adding some kind of reset when 
the calculated idle time returns to 0).
Asserting would mean that the application aborts, which I find a bit overkill 
for a situation that could be handled a lot more elegantly. That would require 
handling the return value from `setupPoller()` in 
`KIdleTimePrivate::loadSystem()` and up, of course.


- René J.V.


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


On Nov. 17, 2015, 10:13 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126078/
> ---
> 
> (Updated Nov. 17, 2015, 10:13 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X, KDE Frameworks and Dario Freddi.
> 
> 
> Repository: kidletime
> 
> 
> Description
> ---
> 
> I noticed that the KIdleTime example doesn't work properly on OS X, and that 
> the plugin for OS X still uses the deprecated Carbon-based algorithm that I 
> already patched for KDE4.
> 
> This patch is a work-in-progress (hence the qDebugs) update to use IOKit, 
> IORegistry and CoreServices to do idle-time calculation as it should be done, 
> and allow simulated user activity through a "less deprecated" function.
> 
> 
> Diffs
> -
> 
>   src/plugins/osx/CMakeLists.txt e1b50b8 
>   src/plugins/osx/macpoller.h ef51ea5 
>   src/plugins/osx/macpoller.cpp ad9c10f 
>   src/plugins/osx/macpoller_helper.mm PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/126078/diff/
> 
> 
> Testing
> ---
> 
> On OS X 10.9 with Qt 5.5.1 and frameworks 5.16.0 .
> 
> The example now works: when I set a QTimer with interval==0, the expected 
> wait for user input (`resumingFromIdle` signal) works. However, I am getting 
> a `stopCatchingIdleEvents` signal which means the application waits forever, 
> without ever getting to compare idle time to the list of timeouts.
> I haven't been able to figure out where that signal comes from, nor why this 
> doesn't happen on Linux.
> 
> Surely I'm missing something, but what?
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: KIdleTime: early and/or failing/rejected timeout detection?

[Aleix Pol]

On Wed, Nov 18, 2015 at 12:33 PM, René J. V.  wrote:
> Actually, there is one thing I may have missed in the documentation, if it's
> there:
>
> Is KIdleTime supposed to detect system idling, or application idling? As it
> happens, it should be possible to make a distinction between the two with the
> latest implementation I made.
>
> As to using an agent to centralise all polling: I think that agent would 
> either
> have to use poll timer with a sufficiently short interval to serve all 
> clients,
> or use a poll timer per current client. Evidently the latter option cancels 
> any
> benefit of moving to an agent-based architecture (leaving only the IPC
> overhead), while the former will lead to worse overhead than what my current
> implementation allows.
> That current implementation is not expensive in its default configuration (and
> that configuration is currently decided at build time), but I think that good
> programming practice would call anyway for deactivating any KIdleTime 
> instances
> when they are not in use, regardless of whether they're based on alarms or on
> polling.
>
> I have no idea how many applications will be using this framework concurrently
> at any given time, but it seems reasonable to expect that there will be less
> than on a system running a KF5 plasma desktop. I see it's used in Baloo, for
> instance, but that framework has dropped support for OS X.
>
> There's also always the possibility to reconsider implementing an Xcb-style
> alarm based mechanism that relies on a helper process, if it turns out in the
> future that KIdleTime/Mac is causing significant amounts of overhead. But
> KIdleTime has changed only minimally from KDE4 to KF5, and it is certainly not
> the case that it has led to any observable amount of continuous overhead until
> now.
>
> R

After a quick search, here's some relevant documentation by Apple:
https://developer.apple.com/library/mac/documentation/Performance/Conceptual/power_efficiency_guidelines_osx/

They suggest you going "App Nap" as soon as possible. Polling from a
framework would ensure this doesn't really happen.

Here's an approach that could maybe inspire you somehow:
http://stackoverflow.com/questions/1419531/mac-screensaver-start-event

Aleix
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126078: [OS X] modernising the KIdleTime plugin (WIP!)

[René J . V . Bertin]

> On Nov. 18, 2015, 8:22 a.m., Martin Gräßlin wrote:
> > src/plugins/osx/macpoller.cpp, lines 222-227
> > 
> >
> > seriously? You care about idle timeouts below 5 msec? This is a 
> > framework to tell the application whether the user doesn't use input 
> > devices. I don't know how fast you type, but I'm relatively certain that it 
> > takes more than 5 msec to move my finger from one key to another. What 
> > exactly is that you want to detect here? An event after each key press, 
> > maybe even between a key press and a key release? Because that's probably 
> > already more about 5 msec.
> 
> René J.V. Bertin wrote:
> The timer is used only for the detection of idle timeouts (so *absence* 
> of user input), and the precision with which it fires determines the accuracy 
> with which those events can be timed. This is currently inaccessible anyway, 
> but I don't believe in doing things half-bakedly. One might question whether 
> it makes sense to allow the framework to work at high precision (I more or 
> less agree that remains to be seen). But I don't think it makes sense to do 
> that and then leave the QTimer in a mode where the requested precision cannot 
> be met.
> 
> Martin Gräßlin wrote:
> To get this quite clear: you care about a precision of below 5 msec, this 
> is a third of the time it takes at least till the next frame is rendered. If 
> you are lucky you get the result of a key press or mouse move represented on 
> screen after 16 msec, more likely it's more. And you care about a wrong timer 
> precision below 5 msec. Sorry that's ridiculous. Please don't include such 
> non-sense code. You make that code more difficult to all other developers to 
> maintain. This code has cost if it's included.
> 
> Aleix Pol Gonzalez wrote:
> It's not a matter of doing things "right" or "wrong". It's a matter of 
> priorities.
> 
> KIdleTime is a framework for figuring out whether the system is idle. I 
> don't consider 5ms not using a system as it's being idling.

Again, we're talking about detecting how long a system has been idle, not 
whether it is (the framework is called KIdle*Time*, right? :)). The precision 
in question here applies just as well to (very) short periods (timeouts) as 
well as to long ones.
Also, don't confound the 5*ms* lower limit under which I switch to a high 
precision timer with the 5`%` error of a Qt::CoarseTimer. The precision of a 
high precision timer may be overkill for longer timeout durations in a 
framework that has a 1ms granularity, but the effect of 5% error scales with 
duration.

I consider that it's up to the user of a feature to know what s/he expects of 
it, not up to a framework. (Turning that around would probably remove the 
raison d'être of a lot of software.)

Finally, I have not been able to measure any increase in overhead when using a 
(single) high precision timer.

Granted, this is a bit a matter of principle for me, but as I've said elsewere, 
I could easily see me having used this in a previous job.


- René J.V.


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


On Nov. 17, 2015, 10:13 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126078/
> ---
> 
> (Updated Nov. 17, 2015, 10:13 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X, KDE Frameworks and Dario Freddi.
> 
> 
> Repository: kidletime
> 
> 
> Description
> ---
> 
> I noticed that the KIdleTime example doesn't work properly on OS X, and that 
> the plugin for OS X still uses the deprecated Carbon-based algorithm that I 
> already patched for KDE4.
> 
> This patch is a work-in-progress (hence the qDebugs) update to use IOKit, 
> IORegistry and CoreServices to do idle-time calculation as it should be done, 
> and allow simulated user activity through a "less deprecated" function.
> 
> 
> Diffs
> -
> 
>   src/plugins/osx/CMakeLists.txt e1b50b8 
>   src/plugins/osx/macpoller.h ef51ea5 
>   src/plugins/osx/macpoller.cpp ad9c10f 
>   src/plugins/osx/macpoller_helper.mm PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/126078/diff/
> 
> 
> Testing
> ---
> 
> On OS X 10.9 with Qt 5.5.1 and frameworks 5.16.0 .
> 
> The example now works: when I set a QTimer with interval==0, the expected 
> wait for user input (`resumingFromIdle` signal) works. However, I am getting 
> a `stopCatchingIdleEvents` signal which means the application waits forever, 
> without ever getting to compare idle time to the list of timeouts.
> I haven't been able to figure out where that signal comes from, nor 

Re: Review Request 126078: [OS X] modernising the KIdleTime plugin (WIP!)

[Martin Gräßlin]

> On Nov. 18, 2015, 8:22 a.m., Martin Gräßlin wrote:
> > src/plugins/osx/macpoller.cpp, lines 222-227
> > 
> >
> > seriously? You care about idle timeouts below 5 msec? This is a 
> > framework to tell the application whether the user doesn't use input 
> > devices. I don't know how fast you type, but I'm relatively certain that it 
> > takes more than 5 msec to move my finger from one key to another. What 
> > exactly is that you want to detect here? An event after each key press, 
> > maybe even between a key press and a key release? Because that's probably 
> > already more about 5 msec.
> 
> René J.V. Bertin wrote:
> The timer is used only for the detection of idle timeouts (so *absence* 
> of user input), and the precision with which it fires determines the accuracy 
> with which those events can be timed. This is currently inaccessible anyway, 
> but I don't believe in doing things half-bakedly. One might question whether 
> it makes sense to allow the framework to work at high precision (I more or 
> less agree that remains to be seen). But I don't think it makes sense to do 
> that and then leave the QTimer in a mode where the requested precision cannot 
> be met.
> 
> Martin Gräßlin wrote:
> To get this quite clear: you care about a precision of below 5 msec, this 
> is a third of the time it takes at least till the next frame is rendered. If 
> you are lucky you get the result of a key press or mouse move represented on 
> screen after 16 msec, more likely it's more. And you care about a wrong timer 
> precision below 5 msec. Sorry that's ridiculous. Please don't include such 
> non-sense code. You make that code more difficult to all other developers to 
> maintain. This code has cost if it's included.
> 
> Aleix Pol Gonzalez wrote:
> It's not a matter of doing things "right" or "wrong". It's a matter of 
> priorities.
> 
> KIdleTime is a framework for figuring out whether the system is idle. I 
> don't consider 5ms not using a system as it's being idling.
> 
> René J.V. Bertin wrote:
> Again, we're talking about detecting how long a system has been idle, not 
> whether it is (the framework is called KIdle*Time*, right? :)). The precision 
> in question here applies just as well to (very) short periods (timeouts) as 
> well as to long ones.
> Also, don't confound the 5*ms* lower limit under which I switch to a high 
> precision timer with the 5`%` error of a Qt::CoarseTimer. The precision of a 
> high precision timer may be overkill for longer timeout durations in a 
> framework that has a 1ms granularity, but the effect of 5% error scales with 
> duration.
> 
> I consider that it's up to the user of a feature to know what s/he 
> expects of it, not up to a framework. (Turning that around would probably 
> remove the raison d'être of a lot of software.)
> 
> Finally, I have not been able to measure any increase in overhead when 
> using a (single) high precision timer.
> 
> Granted, this is a bit a matter of principle for me, but as I've said 
> elsewere, I could easily see me having used this in a previous job.

Rene, please stop it. Don't argue! You won't get this code in with polling and 
setting the qtimer precision based on clearly irrelevant value. It is our 
responsibility as fellow KDE developers to prevent such code to get it. I 
absolutely don't care how you intend to use this code, but this is a user idle 
framework. A system which had an input event in the last five milli seconds is 
not idle per definition. Thus the code is idiotic and wrong. Please stop 
arguing and change the code given the feedback you get here. Same above for the 
nullptr check. That one has to go, it's a wrong check at that place, remove it.


- Martin


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


On Nov. 17, 2015, 10:13 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126078/
> ---
> 
> (Updated Nov. 17, 2015, 10:13 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X, KDE Frameworks and Dario Freddi.
> 
> 
> Repository: kidletime
> 
> 
> Description
> ---
> 
> I noticed that the KIdleTime example doesn't work properly on OS X, and that 
> the plugin for OS X still uses the deprecated Carbon-based algorithm that I 
> already patched for KDE4.
> 
> This patch is a work-in-progress (hence the qDebugs) update to use IOKit, 
> IORegistry and CoreServices to do idle-time calculation as it should be done, 
> and allow simulated user activity through a "less deprecated" function.
> 
> 
> 

Re: Review Request 126078: [OS X] modernising the KIdleTime plugin (WIP!)

[Martin Gräßlin]

> On Nov. 18, 2015, 8:22 a.m., Martin Gräßlin wrote:
> > src/plugins/osx/macpoller.cpp, lines 222-227
> > 
> >
> > seriously? You care about idle timeouts below 5 msec? This is a 
> > framework to tell the application whether the user doesn't use input 
> > devices. I don't know how fast you type, but I'm relatively certain that it 
> > takes more than 5 msec to move my finger from one key to another. What 
> > exactly is that you want to detect here? An event after each key press, 
> > maybe even between a key press and a key release? Because that's probably 
> > already more about 5 msec.
> 
> René J.V. Bertin wrote:
> The timer is used only for the detection of idle timeouts (so *absence* 
> of user input), and the precision with which it fires determines the accuracy 
> with which those events can be timed. This is currently inaccessible anyway, 
> but I don't believe in doing things half-bakedly. One might question whether 
> it makes sense to allow the framework to work at high precision (I more or 
> less agree that remains to be seen). But I don't think it makes sense to do 
> that and then leave the QTimer in a mode where the requested precision cannot 
> be met.

To get this quite clear: you care about a precision of below 5 msec, this is a 
third of the time it takes at least till the next frame is rendered. If you are 
lucky you get the result of a key press or mouse move represented on screen 
after 16 msec, more likely it's more. And you care about a wrong timer 
precision below 5 msec. Sorry that's ridiculous. Please don't include such 
non-sense code. You make that code more difficult to all other developers to 
maintain. This code has cost if it's included.


- Martin


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


On Nov. 17, 2015, 10:13 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126078/
> ---
> 
> (Updated Nov. 17, 2015, 10:13 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X, KDE Frameworks and Dario Freddi.
> 
> 
> Repository: kidletime
> 
> 
> Description
> ---
> 
> I noticed that the KIdleTime example doesn't work properly on OS X, and that 
> the plugin for OS X still uses the deprecated Carbon-based algorithm that I 
> already patched for KDE4.
> 
> This patch is a work-in-progress (hence the qDebugs) update to use IOKit, 
> IORegistry and CoreServices to do idle-time calculation as it should be done, 
> and allow simulated user activity through a "less deprecated" function.
> 
> 
> Diffs
> -
> 
>   src/plugins/osx/CMakeLists.txt e1b50b8 
>   src/plugins/osx/macpoller.h ef51ea5 
>   src/plugins/osx/macpoller.cpp ad9c10f 
>   src/plugins/osx/macpoller_helper.mm PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/126078/diff/
> 
> 
> Testing
> ---
> 
> On OS X 10.9 with Qt 5.5.1 and frameworks 5.16.0 .
> 
> The example now works: when I set a QTimer with interval==0, the expected 
> wait for user input (`resumingFromIdle` signal) works. However, I am getting 
> a `stopCatchingIdleEvents` signal which means the application waits forever, 
> without ever getting to compare idle time to the list of timeouts.
> I haven't been able to figure out where that signal comes from, nor why this 
> doesn't happen on Linux.
> 
> Surely I'm missing something, but what?
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126078: [OS X] modernising the KIdleTime plugin (WIP!)

[Aleix Pol Gonzalez]

> On Nov. 18, 2015, 8:22 a.m., Martin Gräßlin wrote:
> > src/plugins/osx/macpoller.cpp, lines 222-227
> > 
> >
> > seriously? You care about idle timeouts below 5 msec? This is a 
> > framework to tell the application whether the user doesn't use input 
> > devices. I don't know how fast you type, but I'm relatively certain that it 
> > takes more than 5 msec to move my finger from one key to another. What 
> > exactly is that you want to detect here? An event after each key press, 
> > maybe even between a key press and a key release? Because that's probably 
> > already more about 5 msec.
> 
> René J.V. Bertin wrote:
> The timer is used only for the detection of idle timeouts (so *absence* 
> of user input), and the precision with which it fires determines the accuracy 
> with which those events can be timed. This is currently inaccessible anyway, 
> but I don't believe in doing things half-bakedly. One might question whether 
> it makes sense to allow the framework to work at high precision (I more or 
> less agree that remains to be seen). But I don't think it makes sense to do 
> that and then leave the QTimer in a mode where the requested precision cannot 
> be met.
> 
> Martin Gräßlin wrote:
> To get this quite clear: you care about a precision of below 5 msec, this 
> is a third of the time it takes at least till the next frame is rendered. If 
> you are lucky you get the result of a key press or mouse move represented on 
> screen after 16 msec, more likely it's more. And you care about a wrong timer 
> precision below 5 msec. Sorry that's ridiculous. Please don't include such 
> non-sense code. You make that code more difficult to all other developers to 
> maintain. This code has cost if it's included.

It's not a matter of doing things "right" or "wrong". It's a matter of 
priorities.

KIdleTime is a framework for figuring out whether the system is idle. I don't 
consider 5ms not using a system as it's being idling.


- Aleix


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


On Nov. 17, 2015, 10:13 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126078/
> ---
> 
> (Updated Nov. 17, 2015, 10:13 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X, KDE Frameworks and Dario Freddi.
> 
> 
> Repository: kidletime
> 
> 
> Description
> ---
> 
> I noticed that the KIdleTime example doesn't work properly on OS X, and that 
> the plugin for OS X still uses the deprecated Carbon-based algorithm that I 
> already patched for KDE4.
> 
> This patch is a work-in-progress (hence the qDebugs) update to use IOKit, 
> IORegistry and CoreServices to do idle-time calculation as it should be done, 
> and allow simulated user activity through a "less deprecated" function.
> 
> 
> Diffs
> -
> 
>   src/plugins/osx/CMakeLists.txt e1b50b8 
>   src/plugins/osx/macpoller.h ef51ea5 
>   src/plugins/osx/macpoller.cpp ad9c10f 
>   src/plugins/osx/macpoller_helper.mm PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/126078/diff/
> 
> 
> Testing
> ---
> 
> On OS X 10.9 with Qt 5.5.1 and frameworks 5.16.0 .
> 
> The example now works: when I set a QTimer with interval==0, the expected 
> wait for user input (`resumingFromIdle` signal) works. However, I am getting 
> a `stopCatchingIdleEvents` signal which means the application waits forever, 
> without ever getting to compare idle time to the list of timeouts.
> I haven't been able to figure out where that signal comes from, nor why this 
> doesn't happen on Linux.
> 
> Surely I'm missing something, but what?
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126105: Fix yet another crash in Dolphin when Baloo isn't running

[Vishesh Handa]

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

Ship it!


I'm a little hesitant about this, but ship it.

A couple of things though -
1. Dolphin should probably not be sending Baloo so many requests if Baloo is 
disabled. Perhaps we should fix this in Dolphin.
2. This patch is mostly only for the Baloo::File class which causes the db to 
be opened and closed for each operation. It's expensive, but the main use case 
that I focussed on was searching. Over there the db is only opened once per 
application.

- Vishesh Handa


On Nov. 18, 2015, 7:40 p.m., Boudhayan Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126105/
> ---
> 
> (Updated Nov. 18, 2015, 7:40 p.m.)
> 
> 
> Review request for Baloo, KDE Frameworks, Pinak Ahuja, and Vishesh Handa.
> 
> 
> Repository: baloo
> 
> 
> Description
> ---
> 
> Add a check in Baloo::Database::open() to return false if we're opening the 
> database in ReadOnly mode and the database doesn't exist. This fixes a crash 
> in Dolphin when Baloo isn't running.
> 
> This isn't the entire fix - one will also have to remove 
> ~/.local/share/baloo/index to not crash anymore; this patch prevents the file 
> from being recreated everytime Baloo::Database::open() is run, and re-causing 
> the crash.
> 
> 
> Diffs
> -
> 
>   src/engine/database.cpp 4f0579f 
> 
> Diff: https://git.reviewboard.kde.org/r/126105/diff/
> 
> 
> Testing
> ---
> 
> Builds, runs, doesn't crash anymore, the works.
> 
> 
> Thanks,
> 
> Boudhayan Gupta
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125619: Refactor KNewPasswordDialog class

[Christoph Feck]

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

Ship it!



src/knewpassworddialog.h (line 183)


Add @since 5.17 and clarify default color.



src/knewpassworddialog.cpp 


Since you (re)moved the effectivePasswordLenght code, please remove 
"Christoph Feck" from the copyright header.


- Christoph Feck


On Oct. 13, 2015, 1:33 p.m., Elvis Angelaccio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125619/
> ---
> 
> (Updated Oct. 13, 2015, 1:33 p.m.)
> 
> 
> Review request for KDE Frameworks, KDE Usability, Christoph Feck, and David 
> Faure.
> 
> 
> Repository: kwidgetsaddons
> 
> 
> Description
> ---
> 
> Follow-up of the KNewPasswordWidget review.
> By introducing a KNewPasswordWidget in the .ui file, we can:
> 
> - remove the code that has been moved to KNewPasswordWidget
> - enable the KNewPasswordWidget additional features
> 
> The only feature that is currently not available is the ability to remove the 
> strength bar. Do we want to allow developers to show a KNewPasswordDialog 
> without a strenght bar?
> 
> 
> Diffs
> -
> 
>   src/knewpassworddialog.h 1ac7b2151f1e88681a15ce21465449cedb871f1e 
>   src/knewpassworddialog.cpp bd7459acf3c72bc6dc0711da6086213d5111d5c3 
>   src/knewpassworddialog.ui 55a1f62cf4ba6d6c87b2c47742ba3bcd531ebfe8 
> 
> Diff: https://git.reviewboard.kde.org/r/125619/diff/
> 
> 
> Testing
> ---
> 
> Trying to insert a password which is too short or too weak or empty, works as 
> expected.
> 
> 
> File Attachments
> 
> 
> knewpassworddialog1.png
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2015/10/13/62bf21f6-1dcf-46c3-8b1d-146500b5f629__knewpassworddialog1.png
> knewpassworddialog2.png
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2015/10/13/b2e2666f-ce19-4df3-b6ba-25c13fc370ae__knewpassworddialog2.png
> knewpassworddialog3.png
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2015/10/13/b9536088-0cc4-470b-a211-6db4ca79aa2f__knewpassworddialog3.png
> knewpassworddialog4.png
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2015/10/13/4f826716-9ed2-4ecb-8cbe-4fb50abf1086__knewpassworddialog4.png
> 
> 
> Thanks,
> 
> Elvis Angelaccio
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126105: Fix yet another crash in Dolphin when Baloo isn't running

[Sune Vuorela]

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


without actualy knowing the code in question, a brief look over gives me the 
impression that the error handling could be improved and that would make this 
patch not needed.


src/engine/database.cpp (line 104)


SHouldn't there be a if(rc == 0) return false here ?



src/engine/database.cpp (line 137)


if(rc == 0) return false



src/engine/database.cpp (line 167)


if(rc == 0) return false


- Sune Vuorela


On Nov. 18, 2015, 7:40 p.m., Boudhayan Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126105/
> ---
> 
> (Updated Nov. 18, 2015, 7:40 p.m.)
> 
> 
> Review request for Baloo, KDE Frameworks, Pinak Ahuja, and Vishesh Handa.
> 
> 
> Repository: baloo
> 
> 
> Description
> ---
> 
> Add a check in Baloo::Database::open() to return false if we're opening the 
> database in ReadOnly mode and the database doesn't exist. This fixes a crash 
> in Dolphin when Baloo isn't running.
> 
> This isn't the entire fix - one will also have to remove 
> ~/.local/share/baloo/index to not crash anymore; this patch prevents the file 
> from being recreated everytime Baloo::Database::open() is run, and re-causing 
> the crash.
> 
> 
> Diffs
> -
> 
>   src/engine/database.cpp 4f0579f 
> 
> Diff: https://git.reviewboard.kde.org/r/126105/diff/
> 
> 
> Testing
> ---
> 
> Builds, runs, doesn't crash anymore, the works.
> 
> 
> Thanks,
> 
> Boudhayan Gupta
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126105: Fix yet another crash in Dolphin when Baloo isn't running

[Boudhayan Gupta]

> On Nov. 19, 2015, 6:03 a.m., Vishesh Handa wrote:
> > I'm a little hesitant about this, but ship it.
> > 
> > A couple of things though -
> > 1. Dolphin should probably not be sending Baloo so many requests if Baloo 
> > is disabled. Perhaps we should fix this in Dolphin.
> > 2. This patch is mostly only for the Baloo::File class which causes the db 
> > to be opened and closed for each operation. It's expensive, but the main 
> > use case that I focussed on was searching. Over there the db is only opened 
> > once per application.

I concur, the errant calls are being made by Baloo-Widgets and we should 
probably fix Baloo-Widgets to not go through the hassle of invoking lower-level 
baloo functions if it's disabled. I'll look into it - is there a simple API in 
Baloo which I can check to see if indexing is disabled? Can I use 
Baloo::IndexerConfig::fileIndexingEnabled() from Baloo-Widgets to do this check?


- Boudhayan


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


On Nov. 19, 2015, 1:10 a.m., Boudhayan Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126105/
> ---
> 
> (Updated Nov. 19, 2015, 1:10 a.m.)
> 
> 
> Review request for Baloo, KDE Frameworks, Pinak Ahuja, and Vishesh Handa.
> 
> 
> Repository: baloo
> 
> 
> Description
> ---
> 
> Add a check in Baloo::Database::open() to return false if we're opening the 
> database in ReadOnly mode and the database doesn't exist. This fixes a crash 
> in Dolphin when Baloo isn't running.
> 
> This isn't the entire fix - one will also have to remove 
> ~/.local/share/baloo/index to not crash anymore; this patch prevents the file 
> from being recreated everytime Baloo::Database::open() is run, and re-causing 
> the crash.
> 
> 
> Diffs
> -
> 
>   src/engine/database.cpp 4f0579f 
> 
> Diff: https://git.reviewboard.kde.org/r/126105/diff/
> 
> 
> Testing
> ---
> 
> Builds, runs, doesn't crash anymore, the works.
> 
> 
> Thanks,
> 
> Boudhayan Gupta
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126105: Fix yet another crash in Dolphin when Baloo isn't running

[Boudhayan Gupta]

> On Nov. 19, 2015, 1:36 a.m., Thomas Lübking wrote:
> > Errr... you intend to crash applications depending on whether some file is 
> > present??
> > Please fix the actual bug instead of such workaround, got a backtrace?
> 
> Boudhayan Gupta wrote:
> Err... it **doesn't** cause a crash with the patch, causes a crash 
> without it. This is a perfectly valid fix - this function is **supposed** to 
> return false if the database could not be opened, for any reason whatsoever.
> 
> Thomas Lübking wrote:
> I didn't say this patch would introduce a crash.
> I said that "something" crashes for "some" reason and you work around 
> that by testing whether a file is present.
> Your own commit message clearly says that if - for any reason - 
> ~/.local/share/baloo/index exists (eg. i touch it), things will crash again, 
> what means that the actual problem is not resolved but just shadowed.
> 
> Boudhayan Gupta wrote:
> Why would you want to touch ~/.local/share/baloo/index, if you disable 
> baloo?
> 
> Let me explain - balooctl disable works by disabling indexing and 
> removing ~/.local/share/baloo/index. If baloo is disabled, that file is not 
> supposed to exist. A couple of months ago we were even trying to figure out 
> how that file came to exist on systems with Baloo disabled and now I've found 
> out. So this fixes that too.
> 
> Now the real problem. Before this patch, Baloo::Database::open() would 
> just create an empty database. Fair enough, except that db->open() would 
> return true in places where you clearly have an invalid database. This would 
> still work (i.e., not crash), because invalid in this context is empty. 
> However, once you select a ton of files (the number is very weird  - 158 or 
> something) LMDB would scream at you with this problem - **MDB_READERS_FULL: 
> Environment maxreaders limit reached**
> 
> There are two ways to solve this - increase the maxreaders limit in LMDB, 
> or return false if the database doesn't exist. The first option is when you 
> want to go all Linus - *we do not break userspace*, but this way is the more 
> correct way of solving this, because again, on systems where Baloo is 
> disabled, the index file isn't supposed to exist at all.
> 
> I was afraid I was opening up a can of worms by adding yet another 
> condition for db->open() to return false, because I was afraid there were 
> places where a check on db->open()'s return value was missing (and someone 
> would try to do operations on an invalid database). However, I've done all 
> the things that used to trigger crashes before and nothing's crashed yet. 
> With Baloo being both enabled and disabled.
> 
> Thomas Lübking wrote:
> That sounds as if a ton of jobs is started which all try to access the 
> database?
> W/o knowing the details on what is going on, there should probably:
> 
> a) a sane limit per client on how many DB jobs to perform at once 
> (general performance issue)
> b) a sanity check on the index file ie. testing whether it contains some 
> significant bytes or at least isn't empty.
> 
> I guess (b) should actually done by lmdb, but it won't hurt to do it on 
> baloo as well
> 
> Is this also a problem if the file is actually a valid DB, but baloo just 
> disabled?
> 
> 
> ---
> General remark: I guess valid databases should not be deleted but at best 
> be renamed with de/activation, no matter what else happens with this patch.

I'll put this down to a LMDB bug (I suspect some race condition) because when 
the db is active and indexing is enabled, the crash doesn't happen. Therefore I 
see no sense in increasing the limit. As for b), then I'd have to know the LMDB 
file format to do it in Baloo (by default an empty database as created by this 
method is 8KB in size, so there's some sort of data in it).

For your general remark (rename dbs instead of deleting them) - you'd have to 
ask Vishesh. That's a design decision left to the maintainer. I'm just a 
drive-by patcher who's somewhat familiar with the codebase because I've fixed 
quite a few crashes in other apps when Baloo is disabled ;-)


- Boudhayan


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


On Nov. 19, 2015, 1:10 a.m., Boudhayan Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126105/
> ---
> 
> (Updated Nov. 19, 2015, 1:10 a.m.)
> 
> 
> Review request for Baloo, KDE Frameworks, Pinak Ahuja, and Vishesh Handa.
> 
> 
> Repository: baloo
> 
> 
> Description
> ---
> 
> Add a check in Baloo::Database::open() to return false if we're opening the 
> 

Re: Review Request 126105: Fix yet another crash in Dolphin when Baloo isn't running

[Boudhayan Gupta]

> On Nov. 19, 2015, 10:35 a.m., Sune Vuorela wrote:
> > without actualy knowing the code in question, a brief look over gives me 
> > the impression that the error handling could be improved and that would 
> > make this patch not needed.

The assumption here is that if (rc != 0) in normal operation the errors are 
serious enough to warrant crashing the process, not handling it gracefully. 
Granted, I've just discovered one case where this isn't true, but that's when 
indexing is disabled and the db file shouldn't exist, so I've added a special 
check for that.

Also, that patch prevents an empty index from being created even when indexing 
is disabled, which is correct behaviour.


- Boudhayan


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


On Nov. 19, 2015, 1:10 a.m., Boudhayan Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126105/
> ---
> 
> (Updated Nov. 19, 2015, 1:10 a.m.)
> 
> 
> Review request for Baloo, KDE Frameworks, Pinak Ahuja, and Vishesh Handa.
> 
> 
> Repository: baloo
> 
> 
> Description
> ---
> 
> Add a check in Baloo::Database::open() to return false if we're opening the 
> database in ReadOnly mode and the database doesn't exist. This fixes a crash 
> in Dolphin when Baloo isn't running.
> 
> This isn't the entire fix - one will also have to remove 
> ~/.local/share/baloo/index to not crash anymore; this patch prevents the file 
> from being recreated everytime Baloo::Database::open() is run, and re-causing 
> the crash.
> 
> 
> Diffs
> -
> 
>   src/engine/database.cpp 4f0579f 
> 
> Diff: https://git.reviewboard.kde.org/r/126105/diff/
> 
> 
> Testing
> ---
> 
> Builds, runs, doesn't crash anymore, the works.
> 
> 
> Thanks,
> 
> Boudhayan Gupta
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126105: Fix yet another crash in Dolphin when Baloo isn't running

[Thomas Lübking]

> On Nov. 18, 2015, 8:06 nachm., Thomas Lübking wrote:
> > Errr... you intend to crash applications depending on whether some file is 
> > present??
> > Please fix the actual bug instead of such workaround, got a backtrace?
> 
> Boudhayan Gupta wrote:
> Err... it **doesn't** cause a crash with the patch, causes a crash 
> without it. This is a perfectly valid fix - this function is **supposed** to 
> return false if the database could not be opened, for any reason whatsoever.
> 
> Thomas Lübking wrote:
> I didn't say this patch would introduce a crash.
> I said that "something" crashes for "some" reason and you work around 
> that by testing whether a file is present.
> Your own commit message clearly says that if - for any reason - 
> ~/.local/share/baloo/index exists (eg. i touch it), things will crash again, 
> what means that the actual problem is not resolved but just shadowed.
> 
> Boudhayan Gupta wrote:
> Why would you want to touch ~/.local/share/baloo/index, if you disable 
> baloo?
> 
> Let me explain - balooctl disable works by disabling indexing and 
> removing ~/.local/share/baloo/index. If baloo is disabled, that file is not 
> supposed to exist. A couple of months ago we were even trying to figure out 
> how that file came to exist on systems with Baloo disabled and now I've found 
> out. So this fixes that too.
> 
> Now the real problem. Before this patch, Baloo::Database::open() would 
> just create an empty database. Fair enough, except that db->open() would 
> return true in places where you clearly have an invalid database. This would 
> still work (i.e., not crash), because invalid in this context is empty. 
> However, once you select a ton of files (the number is very weird  - 158 or 
> something) LMDB would scream at you with this problem - **MDB_READERS_FULL: 
> Environment maxreaders limit reached**
> 
> There are two ways to solve this - increase the maxreaders limit in LMDB, 
> or return false if the database doesn't exist. The first option is when you 
> want to go all Linus - *we do not break userspace*, but this way is the more 
> correct way of solving this, because again, on systems where Baloo is 
> disabled, the index file isn't supposed to exist at all.
> 
> I was afraid I was opening up a can of worms by adding yet another 
> condition for db->open() to return false, because I was afraid there were 
> places where a check on db->open()'s return value was missing (and someone 
> would try to do operations on an invalid database). However, I've done all 
> the things that used to trigger crashes before and nothing's crashed yet. 
> With Baloo being both enabled and disabled.
> 
> Thomas Lübking wrote:
> That sounds as if a ton of jobs is started which all try to access the 
> database?
> W/o knowing the details on what is going on, there should probably:
> 
> a) a sane limit per client on how many DB jobs to perform at once 
> (general performance issue)
> b) a sanity check on the index file ie. testing whether it contains some 
> significant bytes or at least isn't empty.
> 
> I guess (b) should actually done by lmdb, but it won't hurt to do it on 
> baloo as well
> 
> Is this also a problem if the file is actually a valid DB, but baloo just 
> disabled?
> 
> 
> ---
> General remark: I guess valid databases should not be deleted but at best 
> be renamed with de/activation, no matter what else happens with this patch.
> 
> Boudhayan Gupta wrote:
> I'll put this down to a LMDB bug (I suspect some race condition) because 
> when the db is active and indexing is enabled, the crash doesn't happen. 
> Therefore I see no sense in increasing the limit. As for b), then I'd have to 
> know the LMDB file format to do it in Baloo (by default an empty database as 
> created by this method is 8KB in size, so there's some sort of data in it).
> 
> For your general remark (rename dbs instead of deleting them) - you'd 
> have to ask Vishesh. That's a design decision left to the maintainer. I'm 
> just a drive-by patcher who's somewhat familiar with the codebase because 
> I've fixed quite a few crashes in other apps when Baloo is disabled ;-)

> Therefore I see no sense in increasing the limit.

Not "increase", the opposite - but Vishesh already argued in the same direction 
;-)

It would seem that one of mdb_dbi_open, mdb_dbi_stat or mdb_dbi_flags should 
better return !0 if the database is invalid ...?


- Thomas


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


On Nov. 19, 2015, 7:11 vorm., Boudhayan Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126105/
> 

Re: Review Request 126105: Fix yet another crash in Dolphin when Baloo isn't running

[Boudhayan Gupta]

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

(Updated Nov. 19, 2015, 7:11 a.m.)


Status
--

This change has been marked as submitted.


Review request for Baloo, KDE Frameworks, Pinak Ahuja, and Vishesh Handa.


Changes
---

Submitted with commit 1e880e29883561c8330cb81c03b48716ea68616c by Boudhayan 
Gupta to branch master.


Repository: baloo


Description
---

Add a check in Baloo::Database::open() to return false if we're opening the 
database in ReadOnly mode and the database doesn't exist. This fixes a crash in 
Dolphin when Baloo isn't running.

This isn't the entire fix - one will also have to remove 
~/.local/share/baloo/index to not crash anymore; this patch prevents the file 
from being recreated everytime Baloo::Database::open() is run, and re-causing 
the crash.


Diffs
-

  src/engine/database.cpp 4f0579f 

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


Testing
---

Builds, runs, doesn't crash anymore, the works.


Thanks,

Boudhayan Gupta

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126078: [OS X] modernising the KIdleTime plugin (WIP!)

[Martin Graesslin]

On Wednesday, November 18, 2015 5:28:27 PM CET René J.V. Bertin wrote:
> On Wednesday November 18 2015 15:01:59 Martin Gräßlin wrote:
> > idiotic
> 
> If we're starting to call names arguing indeed becomes pointless as the
> apparent lack of actual reading my arguments already suggested. It leaves
> me with no other choice but to discard the RR and put up a mirror in place.

I want to apologize for saying that. Please be aware that this was only 
intended as a description of code (yes I also call code written by myself 
idiotic and worse things) and not against you.

I hope you don't fork, because I think that would be bad. Not because you move 
the code else where, but because the issues pointed out would be unresolved 
giving users of your code a bad framework and thus harming both your users as 
well as the frameworks project for distributing bad code. The problem of 
polling is real. Please accept this feedback.

Best regards
Martin Gräßlin


signature.asc
Description: This is a digitally signed message part.
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126105: Fix yet another crash in Dolphin when Baloo isn't running

[Thomas Lübking]

> On Nov. 18, 2015, 8:06 nachm., Thomas Lübking wrote:
> > Errr... you intend to crash applications depending on whether some file is 
> > present??
> > Please fix the actual bug instead of such workaround, got a backtrace?
> 
> Boudhayan Gupta wrote:
> Err... it **doesn't** cause a crash with the patch, causes a crash 
> without it. This is a perfectly valid fix - this function is **supposed** to 
> return false if the database could not be opened, for any reason whatsoever.
> 
> Thomas Lübking wrote:
> I didn't say this patch would introduce a crash.
> I said that "something" crashes for "some" reason and you work around 
> that by testing whether a file is present.
> Your own commit message clearly says that if - for any reason - 
> ~/.local/share/baloo/index exists (eg. i touch it), things will crash again, 
> what means that the actual problem is not resolved but just shadowed.
> 
> Boudhayan Gupta wrote:
> Why would you want to touch ~/.local/share/baloo/index, if you disable 
> baloo?
> 
> Let me explain - balooctl disable works by disabling indexing and 
> removing ~/.local/share/baloo/index. If baloo is disabled, that file is not 
> supposed to exist. A couple of months ago we were even trying to figure out 
> how that file came to exist on systems with Baloo disabled and now I've found 
> out. So this fixes that too.
> 
> Now the real problem. Before this patch, Baloo::Database::open() would 
> just create an empty database. Fair enough, except that db->open() would 
> return true in places where you clearly have an invalid database. This would 
> still work (i.e., not crash), because invalid in this context is empty. 
> However, once you select a ton of files (the number is very weird  - 158 or 
> something) LMDB would scream at you with this problem - **MDB_READERS_FULL: 
> Environment maxreaders limit reached**
> 
> There are two ways to solve this - increase the maxreaders limit in LMDB, 
> or return false if the database doesn't exist. The first option is when you 
> want to go all Linus - *we do not break userspace*, but this way is the more 
> correct way of solving this, because again, on systems where Baloo is 
> disabled, the index file isn't supposed to exist at all.
> 
> I was afraid I was opening up a can of worms by adding yet another 
> condition for db->open() to return false, because I was afraid there were 
> places where a check on db->open()'s return value was missing (and someone 
> would try to do operations on an invalid database). However, I've done all 
> the things that used to trigger crashes before and nothing's crashed yet. 
> With Baloo being both enabled and disabled.

That sounds as if a ton of jobs is started which all try to access the database?
W/o knowing the details on what is going on, there should probably:

a) a sane limit per client on how many DB jobs to perform at once (general 
performance issue)
b) a sanity check on the index file ie. testing whether it contains some 
significant bytes or at least isn't empty.

I guess (b) should actually done by lmdb, but it won't hurt to do it on baloo 
as well

Is this also a problem if the file is actually a valid DB, but baloo just 
disabled?


---
General remark: I guess valid databases should not be deleted but at best be 
renamed with de/activation, no matter what else happens with this patch.


- Thomas


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


On Nov. 18, 2015, 7:40 nachm., Boudhayan Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126105/
> ---
> 
> (Updated Nov. 18, 2015, 7:40 nachm.)
> 
> 
> Review request for Baloo, KDE Frameworks, Pinak Ahuja, and Vishesh Handa.
> 
> 
> Repository: baloo
> 
> 
> Description
> ---
> 
> Add a check in Baloo::Database::open() to return false if we're opening the 
> database in ReadOnly mode and the database doesn't exist. This fixes a crash 
> in Dolphin when Baloo isn't running.
> 
> This isn't the entire fix - one will also have to remove 
> ~/.local/share/baloo/index to not crash anymore; this patch prevents the file 
> from being recreated everytime Baloo::Database::open() is run, and re-causing 
> the crash.
> 
> 
> Diffs
> -
> 
>   src/engine/database.cpp 4f0579f 
> 
> Diff: https://git.reviewboard.kde.org/r/126105/diff/
> 
> 
> Testing
> ---
> 
> Builds, runs, doesn't crash anymore, the works.
> 
> 
> Thanks,
> 
> Boudhayan Gupta
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel