Re: Review Request: print-manager on kdereview
Am Mittwoch, 22. August 2012 21:39:11 schrieb Daniel Nicoletti: > Right now translations are mostly undone which is > why I feel important to do the move as soon as possible > so KDE 4.10 has it well translated. Seems there is something wrong with the message extraction in print-manager, because strings from these files are not extracted to a catalog: ./libkcups/KCupsRequest.cpp ./libkcups/KCupsPasswordDialog.cpp ./libkcups/SelectMakeModel.cpp ./libkcups/PPDModel.cpp ./configure-printer/ConfigureDialog.cpp ./configure-printer/PrinterBehavior.cpp ./configure-printer/main.cpp ./configure-printer/ModifyPrinter.cpp ./configure-printer/PrinterOptions.cpp ./printqueue/PrintQueueUi.cpp ./printqueue/main.cpp ./printqueue/PrintQueueModel.cpp ./print-manager-kded/NewPrinterNotification.cpp -- Burkhard Lück
Re: Review Request: print-manager on kdereview
On Wednesday 22 August 2012 21:39:11 Daniel Nicoletti wrote: > Hi list, > > two years ago I started print-manager, at that time > I was using Debian which is affected by this bug: > https://bugs.kde.org/show_bug.cgi?id=271957 > because of this (and the fact I'm not a python fan > to fix the issue) I started print-manager a C++ > implementation that could replace the current > solution and fix the bug I had. Given the fact that the above mentioned bug is about authorization, what does printer-manager do to avoid bug 242648, and how could its methods be transfered to other modules needing KAuth, such as the kdm or clock modules? > * a KDED module to notify the user about new printers > plugged into USB being configured Thanks for caring about blocking D-Bus calls in kded. Hopefully QtDBus is thread-safe :) Will do a more deep review in the coming days. > and please review the code :D which is right now at > https://projects.kde.org/projects/playground/base/print-manager > but a sysadmin request to move has already been filled Sorry, cannot read sysadmin bugs. Where do you want it to move? Christoph Feck (kdepepo) KDE Quality Team
Re: Review Request: print-manager on kdereview
On Thu, Aug 23, 2012 at 11:57 PM, Christoph Feck wrote: > On Wednesday 22 August 2012 21:39:11 Daniel Nicoletti wrote: >> Hi list, >> >> two years ago I started print-manager, at that time >> I was using Debian which is affected by this bug: >> https://bugs.kde.org/show_bug.cgi?id=271957 >> because of this (and the fact I'm not a python fan >> to fix the issue) I started print-manager a C++ >> implementation that could replace the current >> solution and fix the bug I had. > > Given the fact that the above mentioned bug is about authorization, > what does printer-manager do to avoid bug 242648, and how could its > methods be transfered to other modules needing KAuth, such as the kdm > or clock modules? > >> * a KDED module to notify the user about new printers >> plugged into USB being configured > > Thanks for caring about blocking D-Bus calls in kded. Hopefully QtDBus > is thread-safe :) Will do a more deep review in the coming days. > >> and please review the code :D which is right now at >> https://projects.kde.org/projects/playground/base/print-manager >> but a sysadmin request to move has already been filled > > Sorry, cannot read sysadmin bugs. Where do you want it to move? Daniel's request to Sysadmin was only to move it from Playground to KDE Review on projects.kde.org. The statement of intended module wasn't included in that form. > > Christoph Feck (kdepepo) > KDE Quality Team Regards, Ben Cooksley KDE Sysadmin
Re: Review Request: print-manager on kdereview
Sorry I always have problems with these things, I've just copied the messages.sh from my other project Apper, then Kai Uwe b did some changes on it but maybe he missed something.. I'll take a look, but patches are really welcome :D 2012/8/23 Burkhard Lück : > Am Mittwoch, 22. August 2012 21:39:11 schrieb Daniel Nicoletti: >> Right now translations are mostly undone which is >> why I feel important to do the move as soon as possible >> so KDE 4.10 has it well translated. > > Seems there is something wrong with the message extraction in print-manager, > because strings from these files are not extracted to a catalog: > > ./libkcups/KCupsRequest.cpp > ./libkcups/KCupsPasswordDialog.cpp > ./libkcups/SelectMakeModel.cpp > ./libkcups/PPDModel.cpp > ./configure-printer/ConfigureDialog.cpp > ./configure-printer/PrinterBehavior.cpp > ./configure-printer/main.cpp > ./configure-printer/ModifyPrinter.cpp > ./configure-printer/PrinterOptions.cpp > ./printqueue/PrintQueueUi.cpp > ./printqueue/main.cpp > ./printqueue/PrintQueueModel.cpp > ./print-manager-kded/NewPrinterNotification.cpp > > -- > Burkhard Lück -- Daniel Nicoletti KDE Developer - http://dantti.wordpress.com
Re: Review Request: print-manager on kdereview
Am Donnerstag, 23. August 2012 16:49:08 schrieb Daniel Nicoletti: > Sorry I always have problems with these things, > I've just copied the messages.sh from my other > project Apper, then Kai Uwe b did some changes > on it but maybe he missed something.. > Now print-manager has 4 translation catalogs: strings from qml in plasma_applet_printmanager and strings from cpp code in add-printer, kcm_print and print-manager. But all strings in kcm_print and add-printer are extracted to print-manager as well, so from a translators pov the two catalogs kcm_print and add-printer are superfluous and make it hard to keep translations spread over 3 catalog consistent. > I'll take a look, but patches are really welcome :D To provide patches, one needs to know the route you want to go: a) one catalog for all strings from cpp code * remove the Messages.sh in add-printer + kcm_print * and adapt the translation catalog load calls in add-printer + kcm_print b) three different catalogs for strings from cpp code: * exclude extraction from add-printer + kcm_print into catalog print-manager -- Burkhard Lück
Re: Review Request: print-manager on kdereview
2012/8/24 Burkhard Lück : > Am Donnerstag, 23. August 2012 16:49:08 schrieb Daniel Nicoletti: >> Sorry I always have problems with these things, >> I've just copied the messages.sh from my other >> project Apper, then Kai Uwe b did some changes >> on it but maybe he missed something.. >> > Now print-manager has 4 translation catalogs: > > strings from qml in plasma_applet_printmanager > > and > > strings from cpp code in add-printer, kcm_print and print-manager. Wow sorry, I didn't know that there was two other Message.sh there :/ > But all strings in kcm_print and add-printer are extracted to print-manager as > well, so from a translators pov the two catalogs kcm_print and add-printer are > superfluous and make it hard to keep translations spread over 3 catalog > consistent. > >> I'll take a look, but patches are really welcome :D > > To provide patches, one needs to know the route you want to go: > > a) one catalog for all strings from cpp code >* remove the Messages.sh in add-printer + kcm_print >* and adapt the translation catalog load calls in add-printer + kcm_print Ok, I'm going with this, I did already adapt add-printer and kcm_print to use print-manager catalog, but didn't remove the Message.sh script... I hope it's fine now, please check :) > b) three different catalogs for strings from cpp code: > * exclude extraction from add-printer + kcm_print into catalog print-manager > > -- > Burkhard Lück -- Daniel Nicoletti KDE Developer - http://dantti.wordpress.com
Re: Review Request: print-manager on kdereview
Sorry forgot to reply to all.. 2012/8/23 Christoph Feck : > On Wednesday 22 August 2012 21:39:11 Daniel Nicoletti wrote: >> two years ago I started print-manager, at that time >> I was using Debian which is affected by this bug: >> https://bugs.kde.org/show_bug.cgi?id=271957 >> because of this (and the fact I'm not a python fan >> to fix the issue) I started print-manager a C++ >> implementation that could replace the current >> solution and fix the bug I had. > > Given the fact that the above mentioned bug is about authorization, > what does printer-manager do to avoid bug 242648, and how could its > methods be transfered to other modules needing KAuth, such as the kdm > or clock modules? Well CUPS has it's own API for authorization, which is why I avoided the polkit solution s-c-p-gnome now uses. It wasn't easy to make it work right since CUPS API blocks, but it works reliable now that I wrapped it on a thread. >> * a KDED module to notify the user about new printers >> plugged into USB being configured > > Thanks for caring about blocking D-Bus calls in kded. Hopefully QtDBus > is thread-safe :) Will do a more deep review in the coming days. Well I've been moving all my kded modules to threads to avoid those blocks/deadlocks that happened in the past, and till now I had no issue with that. >> and please review the code :D which is right now at >> https://projects.kde.org/projects/playground/base/print-manager >> but a sysadmin request to move has already been filled > > Sorry, cannot read sysadmin bugs. Where do you want it to move? Since the idea is to replace s-c-p-kde it would reside where it is now, so these repos would maybe go to unmaintained: git.kde.org:printer-applet http://websvn.kde.org/trunk/KDE/kdeadmin/system-config-printer-kde/?view=log As the print-manager repo provides both things * a plasmoid and a kded module to replace printer-applet repo * a KCM to replace s-c-p-kde KCM I don't really know the best destination and I would rather not split this repo since it uses an internal lib, maybe kde workspace would be a good place to stay.
Re: Review Request: print-manager on kdereview
On Saturday 25 August 2012 04:29:19 Daniel Nicoletti wrote: > 2012/8/23 Christoph Feck : > > On Wednesday 22 August 2012 21:39:11 Daniel Nicoletti wrote: > >> two years ago I started print-manager, at that time > >> I was using Debian which is affected by this bug: > >> https://bugs.kde.org/show_bug.cgi?id=271957 > >> because of this (and the fact I'm not a python fan > >> to fix the issue) I started print-manager a C++ > >> implementation that could replace the current > >> solution and fix the bug I had. > > > > Given the fact that the above mentioned bug is about > > authorization, what does printer-manager do to avoid bug 242648, > > and how could its methods be transfered to other modules needing > > KAuth, such as the kdm or clock modules? > > Well CUPS has it's own API for authorization, which is why I > avoided the polkit solution s-c-p-gnome now uses. It wasn't easy > to make it work right since CUPS API blocks, but it works reliable > now that I wrapped it on a thread. Okey, so what you do in printer-manager is completely unrelated to kauth. What I am interested in is, how we can use your knowledge to avoid the mistake in the kauth design for frameworks. If I understand mentioned bug correctly, it is not possible to fix the crash with the current design of kauth/polkit interaction, because the polkit frameworks expects the password dialog to run in its own processes. If GNOME still uses polkit, how did they solve the out-of-process password dialogs? > kde workspace would be a good place to stay. Hm, kdeadmin is not moved to git yet, but I guess it still would be possible to create a "KDE Admin" project group. Christoph Feck (kdepepo)
Re: Review Request: print-manager on kdereview
2012/8/25 Christoph Feck : > On Saturday 25 August 2012 04:29:19 Daniel Nicoletti wrote: >> 2012/8/23 Christoph Feck : >> > On Wednesday 22 August 2012 21:39:11 Daniel Nicoletti wrote: >> Well CUPS has it's own API for authorization, which is why I >> avoided the polkit solution s-c-p-gnome now uses. It wasn't easy >> to make it work right since CUPS API blocks, but it works reliable >> now that I wrapped it on a thread. > > Okey, so what you do in printer-manager is completely unrelated to > kauth. > > What I am interested in is, how we can use your knowledge to avoid the > mistake in the kauth design for frameworks. Hmm what kind of mistake are you talking about? I like polkit, but I don't think it fits CUPS design, for printd (a possible CUPS replacement) it will make all sense. > If I understand mentioned bug correctly, it is not possible to fix the > crash with the current design of kauth/polkit interaction, because the > polkit frameworks expects the password dialog to run in its own > processes. It's not exactly a crash, the way s-c-p-kde is working today requires it to run printer-applet as root (which is what some distros do), or have CUPS not to ask password for local admin tasks (Ubuntu case), and in Debian at the time I started print-manager it simply doesn't work. The reason being that it couldn't show a CUPS password dialog and all cups communication was in the GUI thread so it would block.. In other words it wouldn't have an easy fix. > If GNOME still uses polkit, how did they solve the out-of-process > password dialogs? s-c-p-gnome now uses polkit but I don't know exactly how does that work and since CUPS has a method to handle password I think this makes the problem much more complex. >> kde workspace would be a good place to stay. > > Hm, kdeadmin is not moved to git yet, but I guess it still would be > possible to create a "KDE Admin" project group. Right, I'm really not sure about the location, since it's a hardware component I'd say kde workspace under kcontrol dir seems a good place but whatever helps packagers or anything is fine to me :) Best, -- Daniel Nicoletti KDE Developer - http://dantti.wordpress.com
Re: Review Request: print-manager on kdereview
El Divendres, 24 d'agost de 2012, a les 23:29:19, Daniel Nicoletti va escriure: > Sorry forgot to reply to all.. > > 2012/8/23 Christoph Feck : > > On Wednesday 22 August 2012 21:39:11 Daniel Nicoletti wrote: > >> two years ago I started print-manager, at that time > >> I was using Debian which is affected by this bug: > >> https://bugs.kde.org/show_bug.cgi?id=271957 > >> because of this (and the fact I'm not a python fan > >> to fix the issue) I started print-manager a C++ > >> implementation that could replace the current > >> solution and fix the bug I had. > > > > Given the fact that the above mentioned bug is about authorization, > > what does printer-manager do to avoid bug 242648, and how could its > > methods be transfered to other modules needing KAuth, such as the kdm > > or clock modules? > > Well CUPS has it's own API for authorization, which is why I avoided > the polkit solution s-c-p-gnome now uses. It wasn't easy to make it > work right since CUPS API blocks, but it works reliable now that I wrapped > it on a thread. > > >> * a KDED module to notify the user about new printers > >> plugged into USB being configured > > > > Thanks for caring about blocking D-Bus calls in kded. Hopefully QtDBus > > is thread-safe :) Will do a more deep review in the coming days. > > Well I've been moving all my kded modules to threads to avoid those > blocks/deadlocks that happened in the past, and till now I had no issue > with that. > > >> and please review the code :D which is right now at > >> https://projects.kde.org/projects/playground/base/print-manager > >> but a sysadmin request to move has already been filled > > > > Sorry, cannot read sysadmin bugs. Where do you want it to move? > > Since the idea is to replace s-c-p-kde it would reside where it is now, > so these repos would maybe go to unmaintained: > git.kde.org:printer-applet > http://websvn.kde.org/trunk/KDE/kdeadmin/system-config-printer-kde/?view=log You do not seem to be the maintainer of any of these two software. I'm interested in knowing the opinion of the maintainer of them regarding your software kicking them out to unmaintained. Cheers, Albert > > As the print-manager repo provides both things > * a plasmoid and a kded module to replace printer-applet repo > * a KCM to replace s-c-p-kde KCM > I don't really know the best destination and I would rather not > split this repo since it uses an internal lib, maybe > kde workspace would be a good place to stay.
Re: Review Request: print-manager on kdereview
On Mon, Aug 27, 2012 at 07:37:52PM +0200, Albert Astals Cid wrote: > > Since the idea is to replace s-c-p-kde it would reside where it is now, > > so these repos would maybe go to unmaintained: > > git.kde.org:printer-applet > > http://websvn.kde.org/trunk/KDE/kdeadmin/system-config-printer-kde/?view=log > > You do not seem to be the maintainer of any of these two software. I'm > interested in knowing the opinion of the maintainer of them regarding your > software kicking them out to unmaintained. system-config-printer-kde and printer-applet get minimal attention from me alas so I'd be happy enough to have them replaced with something better maintained. print-manager has a few issues but nothing that can't be overcome and we're evaluating shipping it in the next Kubuntu. s-c-p-k has a bunch of issues of its own. Jonathan
Re: Review Request: print-manager on kdereview
On Monday 27 August 2012 19:37:52 Albert Astals Cid wrote: > El Divendres, 24 d'agost de 2012, a les 23:29:19, Daniel Nicoletti > > Since the idea is to replace s-c-p-kde it would reside where it > > is now, so these repos would maybe go to unmaintained: > > git.kde.org:printer-applet > > http://websvn.kde.org/trunk/KDE/kdeadmin/system-config-printer-kd > > e/?view=log > > You do not seem to be the maintainer of any of these two software. > I'm interested in knowing the opinion of the maintainer of them > regarding your software kicking them out to unmaintained. Replacing a Python-based code with (debuggable) C++ code is worth the change alone. You do not want to see the Python backtraces we get... (and yes, Daniel already asked Jonathan :) Christoph Feck (kdepepo)
Re: Review Request: print-manager on kdereview
On Sunday 26 August 2012 06:34:30 Daniel Nicoletti wrote: > 2012/8/25 Christoph Feck : > > On Saturday 25 August 2012 04:29:19 Daniel Nicoletti wrote: > >> 2012/8/23 Christoph Feck : > >> > On Wednesday 22 August 2012 21:39:11 Daniel Nicoletti wrote: > >> Well CUPS has it's own API for authorization, which is why I > >> avoided the polkit solution s-c-p-gnome now uses. It wasn't easy > >> to make it work right since CUPS API blocks, but it works > >> reliable now that I wrapped it on a thread. > > > > Okey, so what you do in printer-manager is completely unrelated > > to kauth. > > > > What I am interested in is, how we can use your knowledge to > > avoid the mistake in the kauth design for frameworks. > > Hmm what kind of mistake are you talking about? I like polkit, but > I don't think it fits CUPS design, for printd (a possible CUPS > replacement) it will make all sense. I am talking about whatever design mistake is responsible for bug 242648, which isn't about CUPS at all. Sorry for side-tracking this thread. I was just thinking that with your expertise on the authorization-related stuff, we could plan better for the future, so that we don't carry this bug over to frameworks. > > If I understand mentioned bug correctly, it is not possible to > > fix the crash with the current design of kauth/polkit > > interaction, because the polkit frameworks expects the password > > dialog to run in its own processes. > > It's not exactly a crash, the way s-c-p-kde is working today > requires it to run printer-applet as root (which is what some > distros do), or have CUPS not to ask password for local admin > tasks (Ubuntu case), and in Debian at the time I started > print-manager it simply doesn't work. The reason being that it > couldn't show a CUPS password dialog and all cups communication > was in the GUI thread so it would block.. In other words it > wouldn't have an easy fix. > > > If GNOME still uses polkit, how did they solve the out-of-process > > password dialogs? > > s-c-p-gnome now uses polkit but I don't know exactly how does that > work and since CUPS has a method to handle password I think this > makes the problem much more complex. > > >> kde workspace would be a good place to stay. > > > > Hm, kdeadmin is not moved to git yet, but I guess it still would > > be possible to create a "KDE Admin" project group. > > Right, I'm really not sure about the location, since it's a > hardware component I'd say kde workspace under kcontrol dir seems > a good place but whatever helps packagers or anything is fine to > me :) > > Best,
Re: Review Request: print-manager on kdereview
2012/8/27 Christoph Feck : > On Sunday 26 August 2012 06:34:30 Daniel Nicoletti wrote: >> 2012/8/25 Christoph Feck : >> > On Saturday 25 August 2012 04:29:19 Daniel Nicoletti wrote: >> >> 2012/8/23 Christoph Feck : >> >> > On Wednesday 22 August 2012 21:39:11 Daniel Nicoletti wrote: >> >> Well CUPS has it's own API for authorization, which is why I >> >> avoided the polkit solution s-c-p-gnome now uses. It wasn't easy >> >> to make it work right since CUPS API blocks, but it works >> >> reliable now that I wrapped it on a thread. >> > >> > Okey, so what you do in printer-manager is completely unrelated >> > to kauth. >> > >> > What I am interested in is, how we can use your knowledge to >> > avoid the mistake in the kauth design for frameworks. >> >> Hmm what kind of mistake are you talking about? I like polkit, but >> I don't think it fits CUPS design, for printd (a possible CUPS >> replacement) it will make all sense. > > I am talking about whatever design mistake is responsible for bug > 242648, which isn't about CUPS at all. Sorry for side-tracking this > thread. I was just thinking that with your expertise on the > authorization-related stuff, we could plan better for the future, so > that we don't carry this bug over to frameworks. Ah ok, it's a completely different issue. Now from the backtrace you already know the issue is due to event loops, I didn't look further but the bugs I had with Apper is because SystemSettings call accept on the open module, then the accept opens an event loop (otherwise returning will close SystemSettings). So to fix this I maybe on KAuth or in SytemSettings a QPoiter needs to check if the event loop is valid. Maybe the KCM could pass an event that can be event->accept() to avoid blocking the call with an event loop. This is going a bit out of the scope of the thread but if you want I can take a closer look to the issue. Best, Daniel
Re: Review Request: print-manager on kdereview
>> Sorry, cannot read sysadmin bugs. Where do you want it to move? > > Since the idea is to replace s-c-p-kde it would reside where it is now, > so these repos would maybe go to unmaintained: > git.kde.org:printer-applet > http://websvn.kde.org/trunk/KDE/kdeadmin/system-config-printer-kde/?view=log > > As the print-manager repo provides both things > * a plasmoid and a kded module to replace printer-applet repo > * a KCM to replace s-c-p-kde KCM > I don't really know the best destination and I would rather not > split this repo since it uses an internal lib, maybe > kde workspace would be a good place to stay. Hi, it's been two weeks since it entered KDE review, I hope all translations issues were fixed, and would like to know where should I move it, and how do we proceed now since it seems no one has objected the replacement. Best, -- Daniel Nicoletti KDE Developer - http://dantti.wordpress.com
Re: Review Request: print-manager on kdereview
El Dijous, 6 de setembre de 2012, a les 18:34:14, Daniel Nicoletti va escriure: > >> Sorry, cannot read sysadmin bugs. Where do you want it to move? > > > > Since the idea is to replace s-c-p-kde it would reside where it is now, > > so these repos would maybe go to unmaintained: > > git.kde.org:printer-applet > > http://websvn.kde.org/trunk/KDE/kdeadmin/system-config-printer-kde/?view=l > > og > > > > As the print-manager repo provides both things > > * a plasmoid and a kded module to replace printer-applet repo > > * a KCM to replace s-c-p-kde KCM > > I don't really know the best destination and I would rather not > > split this repo since it uses an internal lib, maybe > > kde workspace would be a good place to stay. > > Hi, it's been two weeks since it entered KDE review, > I hope all translations issues were fixed, and would like to know > where should I move it, and how do we proceed now since > it seems no one has objected the replacement. Sad noone cared to answer this, i think this is the problem of comiteers, everyone is lazy and waits to see if anyone else will take up the job and do the deicsion. So since noone said anything in 11 days I will, please move it to kdeutils. My rationale is that it replaces stuff from kdeadmin (system-config-printer-kde) and kdeutils (printer-appleat) so the logic would say it shall go to one of the two, since the first is not in git yet, the second one (kdeutils) seems like the best solution. Cheers, Albert > > Best,
Re: Review Request: print-manager on kdereview
El Dilluns, 17 de setembre de 2012, a les 18:45:43, Daniel Nicoletti va escriure: > 2012/9/17 Albert Astals Cid : > > Sad noone cared to answer this, i think this is the problem of comiteers, > > everyone is lazy and waits to see if anyone else will take up the job and > > do the deicsion. > > > > So since noone said anything in 11 days I will, please move it to > > kdeutils. > > > > My rationale is that it replaces stuff from kdeadmin > > (system-config-printer-kde) and kdeutils (printer-appleat) so the logic > > would say it shall go to one of the two, since the first is not in git > > yet, the second one (kdeutils) seems like the best solution. > > Ok, I'll ask the sysadmins to do the move then. Can you please also take care of moving/unmaintaining the two apps it replaces? Albert > > Thanks,
Re: Review Request: print-manager on kdereview
2012/9/17 Albert Astals Cid : > Can you please also take care of moving/unmaintaining the two apps it > replaces? sure, will do. -- Daniel Nicoletti KDE Developer - http://dantti.wordpress.com
KCM Authorization (was: Re: Review Request: print-manager on kdereview)
On Monday 27 August 2012 22:15:18 Daniel Nicoletti wrote: > 2012/8/27 Christoph Feck : > > I am talking about whatever design mistake is responsible for bug > > 242648, which isn't about CUPS at all. Sorry for side-tracking > > this thread. I was just thinking that with your expertise on the > > authorization-related stuff, we could plan better for the > > future, so that we don't carry this bug over to frameworks. > > Ah ok, it's a completely different issue. Now from the backtrace > you already know the issue is due to event loops, I didn't look > further but the bugs I had with Apper is because SystemSettings > call accept on the open module, then the accept opens an event > loop (otherwise returning will close SystemSettings). You already lost me here. What do you mean with "call accept on the module"? > So to fix this I maybe on KAuth or in SytemSettings a QPoiter > needs to check if the event loop is valid. Maybe the KCM could > pass an event that can be event->accept() to avoid blocking the > call with an event loop. Actually, I was always thinking the bug was because there was no blocking. If I follow the bug descriptions correctly, it looks like people can close System Settings, without the password dialog going away, because it is a separate process that isn't informed by KAuth framework while it waits for input. But maybe I am talking stuss ... > This is going a bit out of the scope of the thread but if you want > I can take a closer look to the issue. No need to hurry, it's only 210 duplicates, which is far from crash #1 with 322 duplicates ;) The only problem I see is with triaging. I stopped resolving as a direct duplicate long ago, in order not to flood hundreds of reporters for each additional duplicate, but I do not like this solution (duplicates.cgi still counts them, though). > Best, > Daniel
Re: KCM Authorization (was: Re: Review Request: print-manager on kdereview)
2012/8/27 Christoph Feck :
> On Monday 27 August 2012 22:15:18 Daniel Nicoletti wrote:
>> 2012/8/27 Christoph Feck :
>> > I am talking about whatever design mistake is responsible for bug
>> > 242648, which isn't about CUPS at all. Sorry for side-tracking
>> > this thread. I was just thinking that with your expertise on the
>> > authorization-related stuff, we could plan better for the
>> > future, so that we don't carry this bug over to frameworks.
>>
>> Ah ok, it's a completely different issue. Now from the backtrace
>> you already know the issue is due to event loops, I didn't look
>> further but the bugs I had with Apper is because SystemSettings
>> call accept on the open module, then the accept opens an event
>> loop (otherwise returning will close SystemSettings).
>
> You already lost me here. What do you mean with "call accept on the
> module"?
I'll try to make it clearer:
acceptButton->click();
on_accepted_clicked() {
currentModule->accept();
}
okButton->click();
on_accepted_clicked() {
currentModule->accept();
quit();
}
So to avoid the application quiting modules that need to perform
async tasks like talking to polkit or installing packages (Apper),
they need to create an event loop so that current module accept()
doesn't return (if it does quit() will be called).
So the user clicks Apply, while setting the time or doing anything
the user clicks to close the window, the event loop will return
and some stuff will be already deleted which causes the crashes...
>> So to fix this I maybe on KAuth or in SytemSettings a QPoiter
>> needs to check if the event loop is valid. Maybe the KCM could
>> pass an event that can be event->accept() to avoid blocking the
>> call with an event loop.
>
> Actually, I was always thinking the bug was because there was no
> blocking. If I follow the bug descriptions correctly, it looks like
> people can close System Settings, without the password dialog going
> away, because it is a separate process that isn't informed by KAuth
> framework while it waits for input. But maybe I am talking stuss ...
Yes, the big problem is the user closing SS, because the event loop
will return immediately.
>> This is going a bit out of the scope of the thread but if you want
>> I can take a closer look to the issue.
>
> No need to hurry, it's only 210 duplicates, which is far from crash #1
> with 322 duplicates ;)
hehe well that's a lot of dups, quite boring to keep marking,
unfortunately I'm trying my best to get my commercial app ready to
be sold, so time is short, but if you look at Apper KCM save()
you will see some QPointers to make sure it doesn't crash anymore...
Best,
--
Daniel Nicoletti
KDE Developer - http://dantti.wordpress.com
Re: KCM Authorization (was: Re: Review Request: print-manager on kdereview)
Am 27.08.2012, 22:53 Uhr, schrieb Daniel Nicoletti : So to avoid the application quiting modules that need to perform async tasks like talking to polkit or installing packages (Apper), they need to create an event loop so that current module accept() doesn't return (if it does quit() will be called). So the user clicks Apply, while setting the time or doing anything the user clicks to close the window, the event loop will return and some stuff will be already deleted which causes the crashes... As far as i understood the bottomline is the task to make an external process fully modal to the window, yesno? Aside that the NETWM spec probably SHOULD say that modal windows must not receive input focus and mouse events and also not be closed, weird ideas: a) add an eventfilter on the application to suck all input events (for that window) so clicking apply etc. isn't possible and the window also will not receive close events b) intercept sigterm and reject it as long as you're waiting for the async return Cheers, Thomas
Re: KCM Authorization (was: Re: Review Request: print-manager on kdereview)
2012/8/27 Thomas Lübking : > Am 27.08.2012, 22:53 Uhr, schrieb Daniel Nicoletti : > > >> So to avoid the application quiting modules that need to perform >> async tasks like talking to polkit or installing packages (Apper), >> they need to create an event loop so that current module accept() >> doesn't return (if it does quit() will be called). >> >> So the user clicks Apply, while setting the time or doing anything >> the user clicks to close the window, the event loop will return >> and some stuff will be already deleted which causes the crashes... > > > As far as i understood the bottomline is the task to make an external > process fully modal to the window, yesno? > > Aside that the NETWM spec probably SHOULD say that modal windows must not > receive input focus and mouse events and also not be closed, weird ideas: > a) add an eventfilter on the application to suck all input events (for that > window) so clicking apply etc. isn't possible and the window also will not > receive close events > b) intercept sigterm and reject it as long as you're waiting for the async > return The main problem won't be fixed with this, a kquitapp will still crash the application. (tho maybe b could work but to me seems quite complex...) Nested event loops brings all kinds of trouble... surely there are places where you don't have an option, which is were you must check if the application is quitting.. and in the KCM case only with QPointer I could do this kind of crash go away QAppliction::isClosing() or something like that didn't do the trick (don't recall why)...
Re: KCM Authorization (was: Re: Review Request: print-manager on kdereview)
Am 28.08.2012, 01:56 Uhr, schrieb Daniel Nicoletti : a) add an eventfilter on the application to suck all input events (for that window) so clicking apply etc. isn't possible and the window also will not receive close events b) intercept sigterm and reject it as long as you're waiting for the async return The main problem won't be fixed with this, a kquitapp will still crash the application. Is kquitapp really the main problem? You can hook onto aboutToQuit() and ensure to get rid of the external depends (SIGTERM the password dialog?) but i don't thinks it's possible to block exit(0) calls. (tho maybe b could work but to me seems quite complex...) No. exit(0) does not raise(SIGTERM) or any signal - unfortunately (why kquitapp and exporting exit to dbus - let's say i'd not have done that) Can you point to the troublesome code? Atm i'm frankly not even sure whether the client or the password dialog should manage this (latter could do hardly more than either grab input and ensure it's not passed to the client or sigstop the client - but that does not prevent X11 event processing, they're queued and the client will process them as long as the queue did not get flushwiped on SIGCONT) - also this means that the client won't be able to process anything else interim (including dbus calls) - until it's continued, of course. Cheers, Thomas
Re: KCM Authorization (was: Re: Review Request: print-manager on kdereview)
2012/8/27 Christoph Feck : > On Monday 27 August 2012 22:15:18 Daniel Nicoletti wrote: >> 2012/8/27 Christoph Feck : >> > I am talking about whatever design mistake is responsible for bug >> > 242648, which isn't about CUPS at all. Sorry for side-tracking >> > this thread. I was just thinking that with your expertise on the >> > authorization-related stuff, we could plan better for the >> > future, so that we don't carry this bug over to frameworks. The root of the problem is not in KAuth (which has been already redesigned in Frameworks to be completely async, btw) but in the fact that polkit doesn't conceive a window manager which makes the authorization dialog non-modal. I already tried every possible workaround in 4.x without success, and I honestly didn't test extensively the new architecture in that regard yet. For sure, the main problem won't go away. Consider that KAuth already "hacks" into polkit requests just to set a parent window id.
