Re: Review Request: Enable css border rendering on styled select elements
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104983/ --- (Updated May 22, 2012, 7:14 p.m.) Review request for kdelibs. Changes --- Draw our own drop down arrow so it looks the same regardless the kde style being in use. Description --- The patch enable rendering of css border for styled select elements (combobox+listview). This is done by using KdeUiProxyStyle as already done by other widgets in khtml. For styled combobox the drop down arrow is rendered by drawing PE_IndicatorArrowDown of the base style, so it will look different depending on the system style being in use. This addresses bugs 299934 and 300227. http://bugs.kde.org/show_bug.cgi?id=299934 http://bugs.kde.org/show_bug.cgi?id=300227 Diffs (updated) - khtml/css/html4.css 9f4d17b khtml/rendering/render_form.h 2be4df5 khtml/rendering/render_form.cpp b4cf136 Diff: http://git.reviewboard.kde.org/r/104983/diff/ Testing --- . khtml testregression - no further test fails . tested against different basestyle: oxygen, qtcurve, available qt styles (plastique, qcleanlooks ...) . tested against Qt 4.7.4, Qt 4.8.1 . daily browsing Screenshots --- shoot of a bko page before/after the patch http://git.reviewboard.kde.org/r/104983/s/573/ Thanks, Andrea Iacovitti
Re: Review Request: Apper on kdereview
Hi! This issue has been fixed some time ago. Thanks for the hint! :) Matthias 2012/5/23 Albert Astals Cid aa...@kde.org: El Dilluns, 21 de maig de 2012, a les 18:31:25, Daniel Nicoletti va escriure: Hi, Apper is on playground probably since 2008, it's widely used nowadays so it doesn't make sense to keep it there anymore. So please review the code, make suggestions and such... AppSetup doesn't seem to be loading the apper translation catalog. Albert Right now the code is at (I've asked kde sysadmin to move to kdereview, but afaik it will only reflect the projects url): https://projects.kde.org/projects/playground/sysadmin/apper/repository Thanks :D Daniel
Re: Review Request: Do not restore previously empty location bar URL
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104982/#review14079 --- Ship it! I'm ok with the change, but it seems this area is still buggy in any case... the location bar must be restored to the URL of the content currently displayed doesn't work here, e.g. when typing ~/foo.exe (yes a windows executable, everything else I tried got embedded) in the location bar in a tab that shows www.kde.org, doesn't result in a revert of the location bar url. Generally speaking, I'm not sure that it's such a problem that the location bar doesn't match the contents. This happens all the time -- e.g. when you start typing in the location bar ;) And this even gets saved when switching between tabs, very much on purpose. So maybe we just should not revert the location bar contents, just leave what the user typed. That's what the user in 270723 asks for, that's what my testing with foo.exe shows too (i.e. it's already the current behavior in some cases), and that's consistent with we preserve what the user has typed (already the case currently, when editing the location bar, without hitting Enter, and switching tabs). - David Faure On May 18, 2012, 7:54 a.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104982/ --- (Updated May 18, 2012, 7:54 a.m.) Review request for KDE Base Apps and David Faure. Description --- The attached patch fixes the issue reported in the aforementioned bug report. Specifically, it stops Konqueror from restoring the location bar to the previous URL if a.) It was empty as in a newly created tab. b.) The user typed in a new URL that cannot be handled by Konqueror. The URL was launched with another application. This addresses bug 270723. http://bugs.kde.org/show_bug.cgi?id=270723 Diffs - konqueror/src/konqview.cpp bfeb20b Diff: http://git.reviewboard.kde.org/r/104982/diff/ Testing --- Tested the following three scenarios: 1.) Open a new tab in Konqueror and type-in a URL it cannot handle. 2.) Open a new tab, browse to any location, and then type-in a URL Konqueror cannot handle. 3.) Open a new tab, browse to any location, clear the location bar, and then type-in a URL Konqueror cannot handle. Results with the patch: 1.) The typed in URL remains in the location bar. 2.) The previous URL is restored in the location bar. 3.) Same as #2. Thanks, Dawit Alemayehu
Re: KDE mailing lists - a few questions
Hi, Here's a braindump of the ones I know. On Tuesday, May 22, 2012 09:48:49 Myriam Schweingruber wrote: akademy-sponsoring Private since financial data / sponsoring is being discussed. akademy-talks Discussions among the programme committee are traditionally private among that committee. campkde-organizers Private since financial data / sponsoring is being discussed. community-wg Private since personal things are being discussed. The CWG has a clear privacy policy, please refer to that. dot-editors Strategic / marketing role. ds-announce ds-discuss ds-marketing ds-sponsoring ds-talks ds-team grancanaria k16 Those can probably go away. kde-connect-team Business / financial / strategic stuff, I suppose. kde-enterprise-web kde-ev-campaign Not sure kde-ev-hiring Legal privacy requirement kde-ev-membership Conscious decision by the community kde-events-au Still needed? kde-hci Can go away, most likely. kde-mirrors kde-packager Prereleases of packages, secure channel to distro packagers. kde-pim-meeting kde-pr For private press inquiries kde-press-announce Private by design (contains scoops, non-public information) kde-soc-mentor Private by requirments of SoC kde-webmaster Private for legal requirements (emailing to webmas...@kde.org doesn't indicate the email is being published, we had problems with this in the past) khtml-devel koffice konq-bugs kontact Oversight? mailman Could be security, but dunno. Cheers, -- sebas http://www.kde.org | http://vizZzion.org | GPG Key ID: 9119 0EF9
Re: Review Request: Added option for disabling the offer to save website passwords in Konqueror
On April 26, 2012, 5:09 p.m., Albert Astals Cid wrote:
If i understand you correctly you are suggesting to create a bug (option
that does nothing)?
Doesn't make much sense.
Dawit Alemayehu wrote:
Huh ? I do not follow. By option that does nothing you mean this change
by itself does nothing that is correct. But that is true of every option in
that dialog as well. Moreover, it is a well known fact that you cannot post a
patch for different components in reviewboard. Anyhow, the option will most
definitely be used by kwebkitpart. Whether or not some body implements
support for it in khtml is a question I cannot answer. That is what I meant.
David Faure wrote:
Do you have the kwebkitpart patch ready?
I suppose it'll be easy to apply to khtml as well.
Albert Astals Cid wrote:
You are suggesting to add an option that does nothing in our default html
rendering engine. That is adding a bug. The fact that you know it and don't
care about it makes me sad.
Dawit Alemayehu wrote:
@David: Yes I have a patch for kwebkitpart, but unlike in khtml adding
support for this in kwebkitpart required a very small change in one place in
addition to adding code to read the config option itself in
webkitettings.{h|cpp}. That does not seem to be the case in khtml. It is a
little bit more invovled.
@Albert: Really ?? So how exactly is another browser engine supposed to
provide configuration option to the user if it is supposed to be embedded
into Konqueror ? Why would a developer working on a separate browsing engine
be forced to implement any functionality into khtml ? Would that requirement
apply the other way around ? The last I checked, this is a konqueror setting.
Whether a specific browser engine supports it or not is up to that browser
engine.Just for the record I almost always go out of my way to implement
things in khtml ; especially when I factor out features that are common to
both engines. In this case, I neither have the time nor a complete grasp of
how the KWallet integration works in khtml. As I stated above the change is
not in a single isolated location like it is in kwebkitpart. Feel free to do
a grep in khtml and see for yourself. I can always add the set/get functions
to access the option in KHTMLSettings, but what use would that be if it is
not implemented.
Anyhow, I digress. If there are objections, I will simply move this
feature into the webkit config module I will have to create down the road.
Albert Astals Cid wrote:
So how exactly is another browser engine supposed to provide
configuration option to the user if it is supposed to be embedded into
Konqueror ?
Having it's own engine-only kcm for it's engine-specific options?
Why would a developer working on a separate browsing engine be forced to
implement any functionality into khtml ?
When did i say that?
Would that requirement apply the other way around ?
If you want to use the general apply to all engines options page, of
course.
You probably don't have any bit of user mentallity left in your head,
because it's pretty obvious that adding a configuration option to web
rendering configuration that won't work with our default rendering engine
it's bad usability.
But hey, since David promised to implement this for khtml, go ahead,
don't let me block progress.
Martin Tobias Holmedahl Sandsmark wrote:
I just briefly looked at how complex it would be to implement in KHTML,
and it seems to be enough with this three-line patch?
diff --git a/khtml/ui/passwordbar/storepassbar.cpp
b/khtml/ui/passwordbar/storepassbar.cpp
index 3f5c392..08d79a9 100644
--- a/khtml/ui/passwordbar/storepassbar.cpp
+++ b/khtml/ui/passwordbar/storepassbar.cpp
@@ -80,6 +80,10 @@ StorePass::~StorePass()
void StorePass::saveLoginInformation(const QString host, const QString
key,
const QMapQString, QString walletMap)
{
+ KConfigGroup config( KGlobal::config(), HTML Settings );
+ if (!config.readEntrybool(OfferToSaveWebsitePassword, true))
+return;
+
m_host = host;
m_key = key;
m_walletMap = walletMap;
Dawit Alemayehu wrote:
@Martin: Great. Thanks for the patch.
KHTML patch tested, it works.
- David
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104631/#review12934
---
On April 26, 2012, 3:48 a.m., Dawit Alemayehu wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104631/
---
(Updated April 26, 2012, 3:48 a.m.)
Review request
Re: Review Request: Plasma::RunnerManager: only dequeue our ThreadWeaver jobs
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104973/#review14082 --- Does this have unit tests? Would make sense to add a new one forcing this behaviour? What about docs? Should any doc be fixed/improved mentioning the behaviour? - Albert Astals Cid On May 17, 2012, 1:18 p.m., Aurélien Gâteau wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104973/ --- (Updated May 17, 2012, 1:18 p.m.) Review request for kdelibs. Description --- When RunnerManager::reset() is called, all ThreadWeaver jobs are dequeued, including jobs from other parts of the code. This patch changes this to only dequeue the jobs created by this instance of RunnerManager. Diffs - plasma/runnermanager.cpp 49569a3 Diff: http://git.reviewboard.kde.org/r/104973/diff/ Testing --- I have more than one RunnerManager working at once in a project. Without the patch, only one manager return results. Thanks, Aurélien Gâteau
Re: Review Request: Plasma::RunnerManager: only dequeue our ThreadWeaver jobs
On May 23, 2012, 1:06 p.m., Albert Astals Cid wrote: Does this have unit tests? Would make sense to add a new one forcing this behaviour? What about docs? Should any doc be fixed/improved mentioning the behaviour? There is no test at the moment for the RunnerManager class. Docs does not need to be updated: it's a bug fix, not a behavior change. - Aurélien --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104973/#review14082 --- On May 17, 2012, 1:18 p.m., Aurélien Gâteau wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104973/ --- (Updated May 17, 2012, 1:18 p.m.) Review request for kdelibs. Description --- When RunnerManager::reset() is called, all ThreadWeaver jobs are dequeued, including jobs from other parts of the code. This patch changes this to only dequeue the jobs created by this instance of RunnerManager. Diffs - plasma/runnermanager.cpp 49569a3 Diff: http://git.reviewboard.kde.org/r/104973/diff/ Testing --- I have more than one RunnerManager working at once in a project. Without the patch, only one manager return results. Thanks, Aurélien Gâteau
Re: Review Request: Added option for disabling the offer to save website passwords in Konqueror
On April 26, 2012, 5:09 p.m., Albert Astals Cid wrote:
If i understand you correctly you are suggesting to create a bug (option
that does nothing)?
Doesn't make much sense.
Dawit Alemayehu wrote:
Huh ? I do not follow. By option that does nothing you mean this change
by itself does nothing that is correct. But that is true of every option in
that dialog as well. Moreover, it is a well known fact that you cannot post a
patch for different components in reviewboard. Anyhow, the option will most
definitely be used by kwebkitpart. Whether or not some body implements
support for it in khtml is a question I cannot answer. That is what I meant.
David Faure wrote:
Do you have the kwebkitpart patch ready?
I suppose it'll be easy to apply to khtml as well.
Albert Astals Cid wrote:
You are suggesting to add an option that does nothing in our default html
rendering engine. That is adding a bug. The fact that you know it and don't
care about it makes me sad.
Dawit Alemayehu wrote:
@David: Yes I have a patch for kwebkitpart, but unlike in khtml adding
support for this in kwebkitpart required a very small change in one place in
addition to adding code to read the config option itself in
webkitettings.{h|cpp}. That does not seem to be the case in khtml. It is a
little bit more invovled.
@Albert: Really ?? So how exactly is another browser engine supposed to
provide configuration option to the user if it is supposed to be embedded
into Konqueror ? Why would a developer working on a separate browsing engine
be forced to implement any functionality into khtml ? Would that requirement
apply the other way around ? The last I checked, this is a konqueror setting.
Whether a specific browser engine supports it or not is up to that browser
engine.Just for the record I almost always go out of my way to implement
things in khtml ; especially when I factor out features that are common to
both engines. In this case, I neither have the time nor a complete grasp of
how the KWallet integration works in khtml. As I stated above the change is
not in a single isolated location like it is in kwebkitpart. Feel free to do
a grep in khtml and see for yourself. I can always add the set/get functions
to access the option in KHTMLSettings, but what use would that be if it is
not implemented.
Anyhow, I digress. If there are objections, I will simply move this
feature into the webkit config module I will have to create down the road.
Albert Astals Cid wrote:
So how exactly is another browser engine supposed to provide
configuration option to the user if it is supposed to be embedded into
Konqueror ?
Having it's own engine-only kcm for it's engine-specific options?
Why would a developer working on a separate browsing engine be forced to
implement any functionality into khtml ?
When did i say that?
Would that requirement apply the other way around ?
If you want to use the general apply to all engines options page, of
course.
You probably don't have any bit of user mentallity left in your head,
because it's pretty obvious that adding a configuration option to web
rendering configuration that won't work with our default rendering engine
it's bad usability.
But hey, since David promised to implement this for khtml, go ahead,
don't let me block progress.
Martin Tobias Holmedahl Sandsmark wrote:
I just briefly looked at how complex it would be to implement in KHTML,
and it seems to be enough with this three-line patch?
diff --git a/khtml/ui/passwordbar/storepassbar.cpp
b/khtml/ui/passwordbar/storepassbar.cpp
index 3f5c392..08d79a9 100644
--- a/khtml/ui/passwordbar/storepassbar.cpp
+++ b/khtml/ui/passwordbar/storepassbar.cpp
@@ -80,6 +80,10 @@ StorePass::~StorePass()
void StorePass::saveLoginInformation(const QString host, const QString
key,
const QMapQString, QString walletMap)
{
+ KConfigGroup config( KGlobal::config(), HTML Settings );
+ if (!config.readEntrybool(OfferToSaveWebsitePassword, true))
+return;
+
m_host = host;
m_key = key;
m_walletMap = walletMap;
Dawit Alemayehu wrote:
@Martin: Great. Thanks for the patch.
David Faure wrote:
KHTML patch tested, it works.
Yes, I tested it too and it worked from me as well. I did not commit the change
because the option is really only useful in 4.9.0 so I was waiting until 4.8.4
is tagged and released at the end of this month.
- Dawit
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104631/#review12934
---
On April 26, 2012, 3:48 a.m., Dawit Alemayehu wrote:
Re: KDE mailing lists - a few questions
On Wednesday, May 23, 2012 16:07:48 Myriam Schweingruber wrote: So IMHO these lists should be visible in the mailiman/listinfo, with a description. There already is a lot of criticism about hidden mailing lists with unknown agendas among the KDE community, a minimum of transparency about what list exist and what the lists are about is not asking too much. I think with respect to that, much of it is probably idle conspiracy theorism, don't pay too much attention to it. It's usual the less useful people who find the time to complain about that. (Makes me wonder about their agendas, but in fact -- not really.) Mailinglist which aren't meant for public consumption are not very useful in the list info, that only attracts spammers, adds a lot of overhead for the moderators, and doesn't add anything other than some false sense of transparency (realistically, it achieves the opposite). I understood your email as attempt to sort out the mailinglist mess (is it?), but I'll happily shut up. Cheers, -- sebas http://www.kde.org | http://vizZzion.org | GPG Key ID: 9119 0EF9
Re: KDE mailing lists - a few questions
On Wed, May 23, 2012 at 4:52 PM, Sebastian Kügler se...@kde.org wrote: On Wednesday, May 23, 2012 16:07:48 Myriam Schweingruber wrote: So IMHO these lists should be visible in the mailiman/listinfo, with a description. There already is a lot of criticism about hidden mailing lists with unknown agendas among the KDE community, a minimum of transparency about what list exist and what the lists are about is not asking too much. I think with respect to that, much of it is probably idle conspiracy theorism, don't pay too much attention to it. It's usual the less useful people who find the time to complain about that. (Makes me wonder about their agendas, but in fact -- not really.) Mailinglist which aren't meant for public consumption are not very useful in the list info, that only attracts spammers, adds a lot of overhead for the moderators, and doesn't add anything other than some false sense of transparency (realistically, it achieves the opposite). I am a moderator for several mailing lists, some of these for the fsfe and those have most likely an equal amount of spam traffic than KDE lists. Worse even for the 3 mailing lists I moderate in the *ubuntu space, it sometimes peaks at 30 spam messages a day for one of them, but it is easily handled with listadmin and quite fast, so not something I consider much overhead. Part of the morning routine with a cup of coffee :) A false sense of transparency - I don't see what you mean by that, there already are mailing lists with private archives in the listinfo and I don't see how that would cause people to try to obscure things. But maybe I misunderstand what you mean I understood your email as attempt to sort out the mailinglist mess (is it?)... Yes, that was definitely also part of the plan, thanks for the information about the ones that can be removed. Regards, Myriam -- Proud member of the Amarok and KDE Community Protect your freedom and join the Fellowship of FSFE:done http://www.fsfe.org Please don't send me proprietary file formats, use ISO standard ODF instead (ISO/IEC 26300)
Re: Review Request: Apper on kdereview
El Dimecres, 23 de maig de 2012, a les 13:58:27, Matthias Klumpp va escriure: Hi! This issue has been fixed some time ago. I can't find where the apper catalog loaded in AppSetup, can you point me to it? Cheers, Albert Thanks for the hint! :) Matthias 2012/5/23 Albert Astals Cid aa...@kde.org: El Dilluns, 21 de maig de 2012, a les 18:31:25, Daniel Nicoletti va escriure: Hi, Apper is on playground probably since 2008, it's widely used nowadays so it doesn't make sense to keep it there anymore. So please review the code, make suggestions and such... AppSetup doesn't seem to be loading the apper translation catalog. Albert Right now the code is at (I've asked kde sysadmin to move to kdereview, but afaik it will only reflect the projects url): https://projects.kde.org/projects/playground/sysadmin/apper/repository Thanks :D Daniel
Re: Review Request: Apper on kdereview
Am Montag, 21. Mai 2012, 23:31:25 schrieb Daniel Nicoletti: Hi, Apper is on playground probably since 2008, it's widely used nowadays so it doesn't make sense to keep it there anymore. So please review the code, make suggestions and such... Right now the code is at (I've asked kde sysadmin to move to kdereview, but afaik it will only reflect the projects url): https://projects.kde.org/projects/playground/sysadmin/apper/repository Apper has two man pages, the first is in /Apper, the second is in /AppSetup. Both are not processed by the i18n tool chain aka scripty, but of course should be translated. With the splitted git repos it is already hard enough for i18n + doc team to catch up with documentation locations, but with non predictable directories like in apper that would be a nightmare. Therefore please move both man pages to a new toplevel directory doc as in all other git/svn repos. Thanks. -- Burkhard Lück
Re: KDE mailing lists - a few questions
On Wednesday, May 23, 2012 17:39:52 Myriam Schweingruber wrote: I am a moderator for several mailing lists, some of these for the fsfe and those have most likely an equal amount of spam traffic than KDE lists. Worse even for the 3 mailing lists I moderate in the *ubuntu space, it sometimes peaks at 30 spam messages a day for one of them, but it is easily handled with listadmin and quite fast, so not something I consider much overhead. Part of the morning routine with a cup of coffee Might differ per person, every request for a moderation (even if it's just one lick) causes work, and if necessary, I'd like to avoid it. With mailinglist, those small tasks can pile up quite quickly. A false sense of transparency - I don't see what you mean by that, there already are mailing lists with private archives in the listinfo and I don't see how that would cause people to try to obscure things. But maybe I misunderstand what you mean With that I mean that putting a mailinglist on the list to make things more transparant, only for users to find out that they can't join it is worse than not having the mailinglist listed. In fact, only listing mailinglists with outside relevance makes it much easier to find and pick the right one, as the list to sift through is much shorter and less ambivalent. (Yes, I've explained many people that kde-promo is just fine instead of kde-ev-marketing. Did it make anyone feel better? I doubt it. Can it be easily avoided? Absolutely.) Have a nice evening, -- sebas http://www.kde.org | http://vizZzion.org | GPG Key ID: 9119 0EF9
Re: Review Request: Add Configure Desktop Search button (to open Nepomuk KCM) into Nepomukcontroller Statuswidget
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102523/ --- (Updated May 23, 2012, 3:25 p.m.) Review request for KDE Runtime and Nepomuk. Changes --- Finally came to update (well, rewrite it from scratch) my patch. It now watches on DBus whether the KCM is already opened (randr does the same thing to determin whether it should show that Monitor connected dialog) and hides the Configure button in case. Description --- This little patch adds a Configure Desktop Search button to the nepomukcontroller statuswidget (the little status dialog that appears if you left-click on the nepomuk tray icon). I know that you can access strigi configuration via the tray icon's context menu but I often found myself opening the status dialog and then wanting to get to the config dialog from there. Can't harm, can it? :) I also added a pause/resume icon to the suspend/resume button. Diffs (updated) - nepomuk/kcm/statuswidget.h b7af01e nepomuk/kcm/statuswidget.cpp a9ece31 nepomuk/kcm/statuswidget.ui ab210c8 Diff: http://git.reviewboard.kde.org/r/102523/diff/ Testing --- Thanks, Kai Uwe Broulik
