Re: Using nullptr instead of Q_NULLPTR

[Jarosław Staniek]

Thiago Macieira wrote:

> On Friday 14 August 2015 14:34:49 Jarosław Staniek wrote:
>> I also think we agree that anything other than 0 is good for readability
>> and  readability should be the priority.
> 
> Note that the Qt 5.7 policy still allows and even asks for use of 0 as
> null in some places. The policy is:
> 
>  * always use nullptr in public headers. No exceptions.
>  * use nullptr or 0 in non-public headers or regular sources so
>  readability is
>good. There's no point in using nullptr where 0 is obviously a null
>pointer, like:
> char *s = 0;
> 
>> Then, there's the consistency factor -- a reason to cover the topic of
>> null pointers in the guide. I find neither Qt is consistent in what to
>> use, Qt is not an example here then.
> 
> The policy above applies to Qt 5.7. For 5.6, only the first part applies
> and it needs to be Q_NULLPTR.
> 
>> In the worst case if someone calls us too modern, e.g for embedded
>> projects,  we'd be able a macro to ECM kdecompilersettings that defined
>> nullptr back to 0 (or is this already supported by cmake/etc.?)
> 
> Don't do that. If you use nullptr, there's no going back to zero because
> it's not the same. You've irrevocably locked yourself into requiring a
> compiler that supports it.

I should say "back to Q_NULLPTR", right? 

It's just helping people with old compilers by making the code SC.
 
>> PS: A Krazy check checking for usage of NULLs and Q_NULLPTR would be
>> lovely.  Checking the use of 0's isn't easy, right?
> 
> -Wzero-as-null-pointer-constant
> 
> Qt headers should pass that starting with 5.6.

Yes it's useful for KF5. @everyone: can we have have it added in our build 
flags for KF5 (maybe not for users of KF5)?


-- 
regards, Jaroslaw Staniek

KDE:
: A world-wide network of software engineers, artists, writers, translators
: and facilitators committed to Free Software development - http://kde.org
Calligra Suite:
: A graphic art and office suite - http://calligra.org
Kexi:
: A visual database apps builder - http://calligra.org/kexi
Qt Certified Specialist:
: http://www.linkedin.com/in/jstaniek

Re: Using nullptr instead of Q_NULLPTR

[Jarosław Staniek]

Sergio Martins wrote:

> 
https://community.kde.org/Frameworks/Policies#Frameworks_compiler_requirements_and_C.2B.2B11
> states gcc 4.5 as the minimum version, meaning we can't use nullptr.
> 
> However, since some time now, kf5 libraries are full of nullptr (~400
> occurrences) and nobody noticed.
> 
> We can either:
> - Bump the requirement to gcc 4.6 and allow nullptr
> - Fix kf5 and s/nullptr/Q_NULLPTR
> 
> 

Hi,
I found the the inconsistency in frameworks while thinking about adding some 
null pointer policy for some k*.git repos I maintain.

> I prefer the first option, it's the way forward and if someone was using
> an old gcc he would have complained by now.

This is a good point. This thread drifted in a direction of minimum compiler 
versions supported.
If I read the changelogs correctly msvc is like 8 years ahead of gcc (4.6) 
in this department. MSVC 2005 supported it already, this is enough for us I 
think.

Now in 2015 I propose to go with nullptr. Not using Q_NULLPTR removes on 
point against accepting Qt observed among 3rd-party folks: "Qt guys, like 
the EFL guys, like to invent everything from scratch". A tiny bit but 
always.

I also think we agree that anything other than 0 is good for readability and 
readability should be the priority.

Then, there's the consistency factor -- a reason to cover the topic of null 
pointers in the guide. I find neither Qt is consistent in what to use, Qt is 
not an example here then.

A patch for changing Q_NULLPTRs and NULLs is trivial to make but it's better 
to have it once and not doing further undos, otherwise the git-blame will be 
unnecessarily polluted. A patch for 0's would be a JJ.

In the worst case if someone calls us too modern, e.g for embedded projects, 
we'd be able a macro to ECM kdecompilersettings that defined nullptr back to 
0 (or is this already supported by cmake/etc.?)

Thoughts?

PS: A Krazy check checking for usage of NULLs and Q_NULLPTR would be lovely. 
Checking the use of 0's isn't easy, right?

Maybe I'll use pre-commit hook personally.

> 
> Btw, what are the c++98/c++11 requirements for applications ? I could only
> find the page for frameworks.
> 
> 
> Regards,
> --
> Sérgio Martins
-- 
regards, Jaroslaw Staniek

KDE:
: A world-wide network of software engineers, artists, writers, translators
: and facilitators committed to Free Software development - http://kde.org
Calligra Suite:
: A graphic art and office suite - http://calligra.org
Kexi:
: A visual database apps builder - http://calligra.org/kexi
Qt Certified Specialist:
: http://www.linkedin.com/in/jstaniek

Re: Review Request 120543: Update FindPostgreSQL.cmake

[Jarosław Staniek]

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

(Updated April 2, 2015, 3:02 p.m.)


Status
--

This change has been discarded.


Review request for kdelibs and Alexander Neundorf.


Repository: kdelibs


Description
---

Update FindPostgreSQL.cmake to make is useful. Based on cmake's (3.x) one but 
further improved PostgreSQL_TYPE_INCLUDE_DIR lookup. The fix comes from 
libpredicate (master).

With this improvement, copying FindPostgreSQL.cmake to projects such as 
Calligra/Kexi is no longer needed.


Diffs
-

  cmake/modules/FindPostgreSQL.cmake 7955612 

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


Testing
---

Configuration works for as different include dir setups as openSUSE and Ubuntu. 
Behaviour on Windows/Mac not modified compared to what cmake originally 
provides.


Thanks,

Jarosław Staniek

Review Request 120543: Update FindPostgreSQL.cmake

[Jarosław Staniek]

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

Review request for kdelibs and Alexander Neundorf.


Repository: kdelibs


Description
---

Update FindPostgreSQL.cmake to make is useful. Based on cmake's (3.x) one but 
further improved PostgreSQL_TYPE_INCLUDE_DIR lookup. The fix comes from 
libpredicate (master).

With this improvement, copying FindPostgreSQL.cmake to projects such as 
Calligra/Kexi is no longer needed.


Diffs
-

  cmake/modules/FindPostgreSQL.cmake 7955612 

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


Testing
---

Configuration works for as different include dir setups as openSUSE and Ubuntu. 
Behaviour on Windows/Mac not modified compared to what cmake originally 
provides.


Thanks,

Jarosław Staniek

Re: [RFC] Solution for duplicated or false review/bug notifications

[Jarosław Staniek]

Luigi Toscano wrote:

> Jaroslaw Staniek wrote:
>> tl;dr
>> I propose to treat public KDE git branches (for all repos) having
>> '-work' suffix in a special way: ignore REVIEW and BUG/CCBUG lines.
>> When commit hits a public KDE git branch without -work suffix, current
>> behaviour is preserved.
>> 
>> ** The problem:
>> Whenever commits are pushed from work branches and they contain
>> merged/cherry-picked commits (from other developers) with REVIEW:
>> line, we're getting repeating notes on rb and emails: "This review has
>> been submitted with commit x by  to branch z."
>> I think the hook shall not generate duplicated reviews like this.
> 
>> [...]
>> **What workflow is affected:
>> Affected workflow that use integration with git.reviewboard.kde.org
>> and/or bugs.kde.org is as follows:
>> 
>> 1. Push a change without a REVIEW but optionally with BUG/CCBUG lines.
>> Of course because we do not know the review number at the moment).
>> Pushing to a public work branch is often needed if we expect someone
>> will need/want to test the code.
> 
> 
> So why not using a personal clone for this (publishing the work), and not
> allowing notifications (if it is not the case) for personal clones,
> instead of changing the rules for official repositories?

It's possible but while personal repos are useful, let's see what is
introduced with them in our context:

- for reviewboard I need to reference public project's repository, e.g. 
calligra; otherwise patches will get rejected reviewboard; so before 
submitting to review I need to create a copy of the commit within the 
official repo anyway

- private repo adds another layer in the (already not trivial to explain) 
multiuser workflow: I need to create a copy of the commit in the official 
repo anyway since the personal repo is meant for use (r/w) by the owner (if 
I understand correctly)

- private repos do not support email notification about the changes (hiding 
them was not requested by me)

> Rebase on officially published repository is always bad. I don't think
> officially allowing rebase on a subset (the -work ones) of public branches
> is the proper way to do.

This is not a part of the request. It's about behaviour of integration with 
rb and b.k.o, i.e. KDE-specific integration. When using cherry-picking.

> For cherry-picking, there is not so much to do, as they are new commits.
> Merged commits (not rebased), afaik, do not produce new notifications.

That's true. Our discussion is based on the assumption that cherry-picking 
is valuable just like editing commits before picking them to other branches 
(e.g. split/squash) is. I am askimg my 'students' to commit early and often 
without a fear to their -work branches.

Author of git explains it's jsut another tool which complements merges 
[https://lkml.org/lkml/2008/5/21/351].
But then multiple messages that contain the same REVIEW or BUG lines will 
exist in the repo, when some of them do not mean a review is submitted or a 
bug is resolved. So here's the proposal for addressing this (when needed) in 
advance - when picking a name for a new branch.

There's different solution that addresses the problem of duplicated 
submissions (but not the unwanted premature submissions):
It may be possible to block 'resolving' of already 'resolved' bug on b.k.o 
and submitting already 'submitted' review on rb by altering logics of these 
two sites. 

--
regards / pozdrawiam, Jaroslaw Staniek
 Kexi & Calligra & KDE | http://calligra.org/kexi | http://kde.org
 Qt Certified Specialist | http://qt-project.org
 http://www.linkedin.com/in/jstaniek

Re: Review Request: New Date/Time Widgets in kdelibs/kdeui

[Jarosław Staniek]

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



kdeui/widgets/kdatecombobox.h


missing space before const here and 4 other places



kdeui/widgets/ktimecombobox.cpp


perhaps no need to have m_ prefix for attributes in the private impl class? 
d->m_* looks a bit too verbose



kdeui/widgets/ktimecombobox.cpp


couldn't ki18nc() be used in such places and then KLocalizedString::subs() 
instead of QString::replace()?



- Jarosław


On June 10, 2011, 9:18 p.m., John Layt wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101575/
> ---
> 
> (Updated June 10, 2011, 9:18 p.m.)
> 
> 
> Review request for kdelibs, KDEPIM, KPhotoAlbum, Skrooge, Zanshin, Kevin 
> Ottens, and David Jarvie.
> 
> 
> Summary
> ---
> 
> [Sorry this is a post-commit review and took so long to remember to post. Bad 
> coder, no cookie for you!]
> 
> This review is for some new replacement widgets for the popular KDEPIM 
> KDateEdit and KTimeEdit widgets which were copied into a number of other 
> projects.  These new widgets are clean rewrites (the original widgets have 
> history back to KDE2 days) with slightly changed api but otherwise should 
> replicate the same functionality with a couple of new features.  They will be 
> available for use by any apps using kdelibs 4.7.
> 
> The 3 new widgets are:
> 
> KDateComboBox: A date entry widget derived from KComboBox, the drop-down menu 
> can display a date picker and list of "fancy" dates to choose from.  The list 
> of dates can be configured.
> 
> KTimeComboBox: A time entry widget derived from KComboBox, the drop-down menu 
> can display a list of times to choose from.  The list of times can be 
> configured.
> 
> KDateTimeEdit: A KDateTime entry widget combining KDateComboBox and 
> KTimeComboBox with optional combo's to select the calendar system and time 
> spec as well. This widget should only be used if you want time spec aware 
> data entry.
> 
> All widgets can accept a null or invalid input, it is up to the coder to 
> check for validity of input using isValid() if required.  All feature options 
> of the widgets can be configured.  All widgets can optionally display a 
> warning box on focus out if the entry is invalid.  All widgets can be used in 
> Qt Designer.
> 
> I'm particularly looking for input on the api, and what QWidget event virtual 
> methods I should be reimplementing to make the classes BIC future-proof.
> 
> 
> Diffs
> -
> 
>   kdeui/CMakeLists.txt 1e8b259 
>   includes/KDateComboBox PRE-CREATION 
>   includes/KDateTimeEdit PRE-CREATION 
>   includes/KTimeComboBox PRE-CREATION 
>   includes/CMakeLists.txt 7a8bc5c 
>   kdeui/tests/CMakeLists.txt c7b8026 
>   kdeui/tests/kdatecomboboxtest.h PRE-CREATION 
>   kdeui/tests/kdatecomboboxtest.cpp PRE-CREATION 
>   kdeui/tests/kdatetimeedittest.h PRE-CREATION 
>   kdeui/tests/kdatetimeedittest.cpp PRE-CREATION 
>   kdeui/tests/ktimecomboboxtest.h PRE-CREATION 
>   kdeui/tests/ktimecomboboxtest.cpp PRE-CREATION 
>   kdeui/widgets/kdatecombobox.h PRE-CREATION 
>   kdeui/widgets/kdatecombobox.cpp PRE-CREATION 
>   kdeui/widgets/kdatetimeedit.h PRE-CREATION 
>   kdeui/widgets/kdatetimeedit.cpp PRE-CREATION 
>   kdeui/widgets/kdatetimeedit.ui PRE-CREATION 
>   kdeui/widgets/ktimecombobox.h PRE-CREATION 
>   kdeui/widgets/ktimecombobox.cpp PRE-CREATION 
>   kdewidgets/kde.widgets 9040538 
> 
> Diff: http://git.reviewboard.kde.org/r/101575/diff
> 
> 
> Testing
> ---
> 
> Unit tests written for non-gui functionality.  Gui functionality tested in Qt 
> Designer.  KDateTimeEdit still has a couple of minor bugs, but I didn't want 
> to hold the review up any longer.
> 
> 
> Screenshots
> ---
> 
> Qt Designer Preview
>   http://git.reviewboard.kde.org/r/101575/s/180/
> KDateComboBox
>   http://git.reviewboard.kde.org/r/101575/s/181/
> KTimeComboBox
>   http://git.reviewboard.kde.org/r/101575/s/182/
> 
> 
> Thanks,
> 
> John
> 
>

Re: Review Request: Use QDirIterator in KStandardDirs

[Jarosław Staniek]

> On June 9, 2011, 9:49 a.m., Aaron J. Seigo wrote:
> > my biggest question with this change would be performance. QFileInfo is not 
> > fast, and to make matters "worse" this code in KStandardDirs influences 
> > start up speed. have you measured the performance after this change 
> > relative to before?
> > 
> > with Qt5 this may become a moot issue with the changes coming in the QFile 
> > backends ... but i don't think it makes sense to introduce possible 
> > performance regressions on code that currently works.
> > 
> > is there a platform for which the current code fails?
> 
> Bernhard Beschow wrote:
> I did the patch solely for simplicity and portability reasons, and 
> perhaps (or hopefully) also in the Platform 11 spirit. :) After all, 
> "outsourcing" platform-specific details to Qt wherever we can seems like a 
> good idea to me, such that we can take advantage of new platforms much 
> quicker.
> 
> That said, I couldn't measure any performance difference when logging 
> into a plasma session. In both cases, it takes ~13 secs for the wallpaper to 
> appear, and ~30 secs until the KWallet password dialog appears.

I understand the reasoning but the fact that you did not properly measure the 
differences shows such pieces of code would be better left untouched untill 
there is convincing data. KStandardDirs is critical place for KDE apps. IMHO 
simplifing such lower-level internal (tested!) code may not pay off.

Thanks for your effort.


- Jarosław


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


On June 9, 2011, 7:06 a.m., Bernhard Beschow wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101554/
> ---
> 
> (Updated June 9, 2011, 7:06 a.m.)
> 
> 
> Review request for kdelibs, kdewin and David Faure.
> 
> 
> Summary
> ---
> 
> Increase portability of kdecore a tiny bit by making KStandardDirs use 
> QDirIterator rater than platform-specific code.
> 
> 
> Diffs
> -
> 
>   kdecore/kernel/kstandarddirs.cpp ba90270 
> 
> Diff: http://git.reviewboard.kde.org/r/101554/diff
> 
> 
> Testing
> ---
> 
> I'm running my platform 4.6 with this patch and haven't noticed any 
> regressions yet.
> 
> 
> Thanks,
> 
> Bernhard
> 
>

Re: Review Request: KMessageWidget: Adapt height to text changes

[Jarosław Staniek]

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



kdeui/widgets/kmessagewidget.cpp


Some extra checks needed here.
Sometimes I am getting this warning:

QWidget::setMinimumSize: Negative sizes (0,-1) are not possible
QWidget::setMaximumSize: Negative sizes (16777215,-1) are not possible



- Jarosław


On May 2, 2011, 12:35 p.m., Matthias Fuchs wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101272/
> ---
> 
> (Updated May 2, 2011, 12:35 p.m.)
> 
> 
> Review request for kdelibs and Aurélien Gâteau.
> 
> 
> Summary
> ---
> 
> When there is a long or wrapping text, the text is cut off. This patch fixes 
> that.
> 
> 
> Diffs
> -
> 
>   kdeui/widgets/kmessagewidget.cpp 212ea0d 
> 
> Diff: http://git.reviewboard.kde.org/r/101272/diff
> 
> 
> Testing
> ---
> 
> 
> Screenshots
> ---
> 
> 
>   http://git.reviewboard.kde.org/r/101272/s/154/
> 
>   http://git.reviewboard.kde.org/r/101272/s/155/
> 
> 
> Thanks,
> 
> Matthias
> 
>

Re: Review Request: Add KMessageWidget, an alternative to KMessageBox

[Jarosław Staniek]

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

Ship it!


- Jarosław


On April 29, 2011, 1:44 p.m., Aurélien Gâteau wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101249/
> ---
> 
> (Updated April 29, 2011, 1:44 p.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> ---
> 
> KMessageWidget is a new widget which can be considered as a less intrusive 
> alternative for KMessageBox. It was designed during KDE UX sprint 2011 ( 
> http://community.kde.org/Sprints/UX2011/KMessageWidget ).
> 
> The class documentation should make it clear how and when it can be used.
> 
> This widget could be used by:
> - Browsers to implement their "remember password" widgets (Konqueror, 
> Rekonq...)
> - Forms to provide feedback on validation errors
> - Bluedevil KCM to replace its custom "No Bluetooth adapter have been found" 
> message widget
> - Nepomuk/Strigi KCM to indicate status of their services
> - Gwenview to replace its custom save bar
> - ...
> 
> I still have a few additions in mind for the API but it is already usable and 
> since we are freezing I think it can be merged in master in its current 
> state. I assume it will still be possible to extend the API a bit before 
> kdelibs 4.7 freezes for good.
> 
> I also wrote an example program in the kdeexample repository ( 
> https://projects.kde.org/projects/kde/kdeexamples/repository/show?rev=agateau%2Fkmessagewidget
>  ), though I am wondering whether it shouldn't be moved in kdeui/tests as it 
> is more a manual test program than an example.
> 
> 
> Diffs
> -
> 
>   kdeui/CMakeLists.txt d1873d1 
>   kdeui/widgets/kmessagewidget.h PRE-CREATION 
>   kdeui/widgets/kmessagewidget.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/101249/diff
> 
> 
> Testing
> ---
> 
> 
> Screenshots
> ---
> 
> Montage from kmessagewidgetdemo
>   http://git.reviewboard.kde.org/r/101249/s/141/
> 
> 
> Thanks,
> 
> Aurélien
> 
>

Re: Review Request: Add KMessageWidget, an alternative to KMessageBox

[Jarosław Staniek]

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


Ship it!

PS: the callouts I have implemented: 
http://simplest-image-hosting.net/jpg-0-plasma-desktophm4013
It's BC with your API.

- Jarosław


On April 29, 2011, 1:44 p.m., Aurélien Gâteau wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101249/
> ---
> 
> (Updated April 29, 2011, 1:44 p.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> ---
> 
> KMessageWidget is a new widget which can be considered as a less intrusive 
> alternative for KMessageBox. It was designed during KDE UX sprint 2011 ( 
> http://community.kde.org/Sprints/UX2011/KMessageWidget ).
> 
> The class documentation should make it clear how and when it can be used.
> 
> This widget could be used by:
> - Browsers to implement their "remember password" widgets (Konqueror, 
> Rekonq...)
> - Forms to provide feedback on validation errors
> - Bluedevil KCM to replace its custom "No Bluetooth adapter have been found" 
> message widget
> - Nepomuk/Strigi KCM to indicate status of their services
> - Gwenview to replace its custom save bar
> - ...
> 
> I still have a few additions in mind for the API but it is already usable and 
> since we are freezing I think it can be merged in master in its current 
> state. I assume it will still be possible to extend the API a bit before 
> kdelibs 4.7 freezes for good.
> 
> I also wrote an example program in the kdeexample repository ( 
> https://projects.kde.org/projects/kde/kdeexamples/repository/show?rev=agateau%2Fkmessagewidget
>  ), though I am wondering whether it shouldn't be moved in kdeui/tests as it 
> is more a manual test program than an example.
> 
> 
> Diffs
> -
> 
>   kdeui/CMakeLists.txt d1873d1 
>   kdeui/widgets/kmessagewidget.h PRE-CREATION 
>   kdeui/widgets/kmessagewidget.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/101249/diff
> 
> 
> Testing
> ---
> 
> 
> Screenshots
> ---
> 
> Montage from kmessagewidgetdemo
>   http://git.reviewboard.kde.org/r/101249/s/141/
> 
> 
> Thanks,
> 
> Aurélien
> 
>

Re: Review Request: Add KMessageWidget, an alternative to KMessageBox

[Jarosław Staniek]

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



kdeui/widgets/kmessagewidget.cpp


please change '<' with '&', then the result is what we mean...


- Jarosław


On April 29, 2011, 1:44 p.m., Aurélien Gâteau wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101249/
> ---
> 
> (Updated April 29, 2011, 1:44 p.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> ---
> 
> KMessageWidget is a new widget which can be considered as a less intrusive 
> alternative for KMessageBox. It was designed during KDE UX sprint 2011 ( 
> http://community.kde.org/Sprints/UX2011/KMessageWidget ).
> 
> The class documentation should make it clear how and when it can be used.
> 
> This widget could be used by:
> - Browsers to implement their "remember password" widgets (Konqueror, 
> Rekonq...)
> - Forms to provide feedback on validation errors
> - Bluedevil KCM to replace its custom "No Bluetooth adapter have been found" 
> message widget
> - Nepomuk/Strigi KCM to indicate status of their services
> - Gwenview to replace its custom save bar
> - ...
> 
> I still have a few additions in mind for the API but it is already usable and 
> since we are freezing I think it can be merged in master in its current 
> state. I assume it will still be possible to extend the API a bit before 
> kdelibs 4.7 freezes for good.
> 
> I also wrote an example program in the kdeexample repository ( 
> https://projects.kde.org/projects/kde/kdeexamples/repository/show?rev=agateau%2Fkmessagewidget
>  ), though I am wondering whether it shouldn't be moved in kdeui/tests as it 
> is more a manual test program than an example.
> 
> 
> Diffs
> -
> 
>   kdeui/CMakeLists.txt d1873d1 
>   kdeui/widgets/kmessagewidget.h PRE-CREATION 
>   kdeui/widgets/kmessagewidget.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/101249/diff
> 
> 
> Testing
> ---
> 
> 
> Screenshots
> ---
> 
> Montage from kmessagewidgetdemo
>   http://git.reviewboard.kde.org/r/101249/s/141/
> 
> 
> Thanks,
> 
> Aurélien
> 
>

Re: GeoClue dependencies

[Jarosław Staniek]

Kevin Krammer wrote:

> On Thursday, 2011-04-28, John Layt wrote:
>> Hi,
>> 
>> There's been a short discussion on the GeoClue mailing list related to
>> resolving the issues we have around their dependencies on gconf and
>> gsettings
>> 
>> and the latest response has been:
>> > The GConf dependency is already gone. And I wouldn't take a patch to
>> > remove the GSettings dependency. There's Qt bindings to access
>> > GSettings, and GSettings lives in GIO, which is also where the
>> > dbus-glib replacement (GDBus) lives. I don't think that trying to
>> > replace a library that's already in the dependencies due to the way
>> > packages are built is buying us anything but too moving parts.
>> 
>> Now, I don't really know about GSettings, but I'm guessing this isn't an
>> acceptable things for us?
> 
> Can you elaborate on how this dependency affects KDE? Isn't GeoClue
> offering its service through D-Bus interfaces?

IIRC it is, not only actual service is available but set of providers are 
introspectable and option getters/setters are available via dbus. 

http://folks.o-hand.com/jku/geoclue-docs/Geoclue.html

A nightmare for me if using via dbus C API is used, but with Qt...

-- 
regards / pozdrawiam, Jaroslaw Staniek
 http://www.linkedin.com/in/jstaniek
 Kexi & Calligra (kexi-project.org, identi.ca/kexi, calligra-suite.org)
 KDE Software Development Platform on MS Windows (windows.kde.org)

Re: Review Request: Add KMessageWidget, an alternative to KMessageBox

[Jarosław Staniek]

> On April 28, 2011, 8:07 p.m., Jarosław Staniek wrote:
> > I admit I reviewed the code extensively since I used it in Kexi already, 
> > extending by adding default action feature, callouts, and autodeletion. 
> > Good job!
> > 
> > Please look at these supplementary notes too - I'd like to start discussion:
> > There's possible problem with adding new widgets in so matured API as 
> > KDElibs 4.7: I do not know how we'll deal with the problem of supporting 
> > KDE 4.6 apps that require this new widget (without forking). As I said on 
> > kde-core-devel, ideally new widgets would land in extra library and could 
> > be backported. Also this widget has almost nothing to do with 
> > KDElibs-sepcific features and some peopel would _love_ to use it in non-KDE 
> > code too. Again, without forking.

BTW, links to my extensions:
http://quickgit.kde.org/?p=calligra.git&a=blob&hb=60e7ff67fee20723a4a318339d982c85253307a5&f=kexi/kexiutils/kmessagewidget.h
http://quickgit.kde.org/?p=calligra.git&a=blob&h=67d11d15bd8c7df894ea7b048aeb9306efb7a1ab&hb=60e7ff67fee20723a4a318339d982c85253307a5&f=kexi/kexiutils/kmessagewidget.cpp


- Jarosław


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


On April 28, 2011, 3:31 p.m., Aurélien Gâteau wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101249/
> ---
> 
> (Updated April 28, 2011, 3:31 p.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> ---
> 
> KMessageWidget is a new widget which can be considered as a less intrusive 
> alternative for KMessageBox. It was designed during KDE UX sprint 2011 ( 
> http://community.kde.org/Sprints/UX2011/KMessageWidget ).
> 
> The class documentation should make it clear how and when it can be used.
> 
> This widget could be used by:
> - Browsers to implement their "remember password" widgets (Konqueror, 
> Rekonq...)
> - Forms to provide feedback on validation errors
> - Bluedevil KCM to replace its custom "No Bluetooth adapter have been found" 
> message widget
> - Nepomuk/Strigi KCM to indicate status of their services
> - Gwenview to replace its custom save bar
> - ...
> 
> I still have a few additions in mind for the API but it is already usable and 
> since we are freezing I think it can be merged in master in its current 
> state. I assume it will still be possible to extend the API a bit before 
> kdelibs 4.7 freezes for good.
> 
> I also wrote an example program in the kdeexample repository ( 
> https://projects.kde.org/projects/kde/kdeexamples/repository/show?rev=agateau%2Fkmessagewidget
>  ), though I am wondering whether it shouldn't be moved in kdeui/tests as it 
> is more a manual test program than an example.
> 
> 
> Diffs
> -
> 
>   kdeui/CMakeLists.txt d1873d1 
>   kdeui/widgets/kmessagewidget.h PRE-CREATION 
>   kdeui/widgets/kmessagewidget.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/101249/diff
> 
> 
> Testing
> ---
> 
> 
> Screenshots
> ---
> 
> Montage from kmessagewidgetdemo
>   http://git.reviewboard.kde.org/r/101249/s/141/
> 
> 
> Thanks,
> 
> Aurélien
> 
>

Re: Review Request: Add KMessageWidget, an alternative to KMessageBox

[Jarosław Staniek]

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

Ship it!


I admit I reviewed the code extensively since I used it in Kexi already, 
extending by adding default action feature, callouts, and autodeletion. Good 
job!

Please look at these supplementary notes too - I'd like to start discussion:
There's possible problem with adding new widgets in so matured API as KDElibs 
4.7: I do not know how we'll deal with the problem of supporting KDE 4.6 apps 
that require this new widget (without forking). As I said on kde-core-devel, 
ideally new widgets would land in extra library and could be backported. Also 
this widget has almost nothing to do with KDElibs-sepcific features and some 
peopel would _love_ to use it in non-KDE code too. Again, without forking.


kdeui/widgets/kmessagewidget.h


How about having Q_Properties ?



kdeui/widgets/kmessagewidget.h


It would be good to add ctor KMessageWidget(const QString* text, parent) 
ctor too for convenience, it's frequent case



kdeui/widgets/kmessagewidget.h


setCloseButtonVisible() would be better than verb show

Also isCloseButtonVisible() setter needed for completness

Also it would be good to make setCloseButtonVisible() a slot since other 
property (wordWrap) has setter as slot






kdeui/widgets/kmessagewidget.cpp


Do we need this attribute? labelText->text() has the same information.



kdeui/widgets/kmessagewidget.cpp


I suggest adding 
 textLabel->setTextInteractionFlags(Qt::TextBrowserInteraction);

we use such flag in KMessageBox too, this enables links and text selection, 
useful when error copying texts by users



kdeui/widgets/kmessagewidget.cpp


Please change to SimpleAnimationEffects, since in other places this kind of 
animations are allowed for SimpleAnimationEffects level, e.g.: 
KFadeWidgetEffect.


- Jarosław


On April 28, 2011, 3:31 p.m., Aurélien Gâteau wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101249/
> ---
> 
> (Updated April 28, 2011, 3:31 p.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> ---
> 
> KMessageWidget is a new widget which can be considered as a less intrusive 
> alternative for KMessageBox. It was designed during KDE UX sprint 2011 ( 
> http://community.kde.org/Sprints/UX2011/KMessageWidget ).
> 
> The class documentation should make it clear how and when it can be used.
> 
> This widget could be used by:
> - Browsers to implement their "remember password" widgets (Konqueror, 
> Rekonq...)
> - Forms to provide feedback on validation errors
> - Bluedevil KCM to replace its custom "No Bluetooth adapter have been found" 
> message widget
> - Nepomuk/Strigi KCM to indicate status of their services
> - Gwenview to replace its custom save bar
> - ...
> 
> I still have a few additions in mind for the API but it is already usable and 
> since we are freezing I think it can be merged in master in its current 
> state. I assume it will still be possible to extend the API a bit before 
> kdelibs 4.7 freezes for good.
> 
> I also wrote an example program in the kdeexample repository ( 
> https://projects.kde.org/projects/kde/kdeexamples/repository/show?rev=agateau%2Fkmessagewidget
>  ), though I am wondering whether it shouldn't be moved in kdeui/tests as it 
> is more a manual test program than an example.
> 
> 
> Diffs
> -
> 
>   kdeui/CMakeLists.txt d1873d1 
>   kdeui/widgets/kmessagewidget.h PRE-CREATION 
>   kdeui/widgets/kmessagewidget.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/101249/diff
> 
> 
> Testing
> ---
> 
> 
> Screenshots
> ---
> 
> Montage from kmessagewidgetdemo
>   http://git.reviewboard.kde.org/r/101249/s/141/
> 
> 
> Thanks,
> 
> Aurélien
> 
>