Re: Noteworthy changes when porting to C++17

[Ivan Čukić]

> > What I have seen is that std::mem_fun was used within KIO and has been
> > replaced by std::mem_fn. Not sure if that counts as "commonly used",
> > though.

> Imo, usages of either of these two should be rewritten to use lambdas
> instead.

-1

Member function pointers are more readable than lambdas in cases
where they can be used (std::ranges). Sadly, mem_fn is a syntactic noise
meant as an excuse why some things don't accept all invokables/callables
but only function objects. But, while it is uglier than just a pointer to a 
member function, I still find it more readable then lambdas. And it is 
optimized out by good compilers (tested on gcc 10) just as lambdas are.

std::ranges::all_of(vs, &some_t::check);

versus

std::all_of(..., std::mem_fn(&some_t::check));

versus

std::all_of(..., [] (const some_t& t) {
    return t.check();
});

Cheers,
Ivan

-- 
dr Ivan Čukić
i...@cukic.co, https://cukic.co/
gpg key fingerprint: 8FE4 D32F 7061 EA9C 8232  07AE 01C6 CE2B FF04 1C12

Re: Information regarding upcoming Gitlab Migration: clarifications

[Ivan Čukić]

> We have made a big fuss in the past about having different projects
> that do the same thing and now we'll have that but also we'll have
> several projects with the same name?
> It really feels off to me and I wonder if this is related to the move to
> gitlab.

+1 to both sentiments - that projects should have different names and that 
this is a bit off topic for the gitlab migration.

Cheers,
Ivan



-- 
dr Ivan Čukić
i...@cukic.co, https://cukic.co/
gpg key fingerprint: 8FE4 D32F 7061 EA9C 8232  07AE 01C6 CE2B FF04 1C12

Re: Update on Status of Gitlab Migration

[Ivan Čukić]

Hi all,

While I do like the invent name, I agree there should be a redirect of some 
sort from git.kde.org to it.

The aforementioned Debian salsa server has one as well - https://
git.debian.org/ - a message stating that the new server is 'salsa.debian.org'

Cheers,
Ivan


-- 
dr Ivan Čukić
i...@cukic.co, https://cukic.co/
gpg key fingerprint: 8FE4 D32F 7061 EA9C 8232  07AE 01C6 CE2B FF04 1C12

D28746: Show previews on encrypted filesystems

[Ivan Čukić]

ivan accepted this revision.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D28746

To: marcingu, ivan, #frameworks, dfaure, ngraham
Cc: ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

D28745: Skipping catching of thumbnails on encrypted filesystems

[Ivan Čukić]

ivan accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D28745

To: marcingu, ivan, broulik, #dolphin
Cc: kde-frameworks-devel, kfm-devel, azyx, nikolaik, pberestov, iasensio, 
fprice, LeGast00n, cblack, fbampaloukas, alexde, Codezela, feverfew, meven, 
michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, rdieter, mikesomov

D28746: Show previews on encrypted filesystems

[Ivan Čukić]

ivan added inline comments.

INLINE COMMENTS

> previewjob.cpp:697
>   
> !currentItem.item.url().adjusted(QUrl::RemoveFilename).toLocalFile().startsWith(thumbRoot))
> -&& !sequenceIndex;
> +&& !sequenceIndex && !isEncrypted;
>  QImage thumb;

Since you are already changing this, move these two checks to the line after 
`bSave` - short-circuit logic to skip calling `->property` and the rest if 
these two are not satisfied.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D28746

To: marcingu, ivan, #frameworks, dfaure, ngraham
Cc: ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

D28746: Show previews on encrypted filesystems

[Ivan Čukić]

ivan added inline comments.

INLINE COMMENTS

> previewjob.cpp:692
>  {
> +bool isEncrypted = 
> encryptedMountsList.findByPath(currentItem.item.url().toLocalFile());
>  bool save = bSave &&

`const`

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D28746

To: marcingu, ivan, #frameworks, dfaure, ngraham
Cc: ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

D28745: Skipping catching of thumbnails on encrypted filesystems

[Ivan Čukić]

ivan requested changes to this revision.
ivan added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> thumbnail.cpp:730
> +const auto thumbRootMount = mountsList.findByPath(thumbRoot);
> +std::copy_if(mountsList.begin(), mountsList.end(),
> + std::back_inserter(encryptedMountsList),

Seems suboptimal to copy all the items just to be able to check whether ant of 
them satisfy a requirement - `std::any_of` could be an alternative.

Or, possibly even better, `mountsList.findByPath(...)` and then check the 
filesystem type.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D28745

To: marcingu, ivan, broulik, #dolphin
Cc: kde-frameworks-devel, kfm-devel, azyx, nikolaik, pberestov, iasensio, 
fprice, LeGast00n, cblack, fbampaloukas, alexde, Codezela, feverfew, meven, 
michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, rdieter, mikesomov

D24401: Use exposed DBus methods to switch activities in CLI

[Ivan Čukić]

ivan accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R6 KActivities

BRANCH
  cli-nextprev-activity (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D24401

To: muesli, ivan
Cc: ngraham, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, bruns

D26798: Fix broken SQL query in allResourcesQuery

[Ivan Čukić]

ivan accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R159 KActivities Statistics

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D26798

To: meven, ivan, #frameworks
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

Re: KApplicationTrader API

[Ivan Čukić]

> auto filter = [](const KService::Ptr &service) { return
> service->name().contains("km", Qt::CaseInsensitive); } auto offers =
> KApplicationTrader::self()->query(filter);

I'd vote for lambda/function object/callable (hello std::invoke in KF6?). If a 
more terse syntax is needed, it could be later provided with an eDSL along the 
lines of:

KApplicationTrader::query(name.contains("km", Qt::CaseInsensitive));
KApplicationTrader::query(
filter::name.endsWith("km") &&
filter::showInKDE);


> auto offers = KApplicationTrader::query(...);

+1, no need to put everything in an object.


> Note that the least-porting-effort solution is self() and trader language,
> since the code currently uses KServiceTypeTrader for this.

I'm aware I voted for the most-porting-effort solution ;)


Cheers,
Ivan


-- 
dr Ivan Čukić
KDE, ivan.cu...@kde.org, https://cukic.co/
gpg key fingerprint: 8FE4 D32F 7061 EA9C 8232  07AE 01C6 CE2B FF04 1C12

D26415: Ensure to store resource uri without a trailing slash

[Ivan Čukić]

ivan accepted this revision.
ivan added a comment.
This revision is now accepted and ready to land.


  TBH, I'd like more to have dirs always end with a slash, but I guess this is 
fine as well.

REPOSITORY
  R6 KActivities

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D26415

To: meven, ivan, #frameworks
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24741: Add a utility accessor to get a QUrl from a ResultSet::Result

[Ivan Čukić]

ivan accepted this revision.
ivan added a comment.
This revision is now accepted and ready to land.


  Just add TODO and you are free to push

INLINE COMMENTS

> meven wrote in resultset.h:78
> `url` makes more sense to me, no need to decorate it, this is idiomatic 
> KDE/Qt. `toUrl` might make sense alternatively since it is not a free 
> operation as it is a copy.
> 
> I am not too aware of the history around Nepomuk.
> 
> File paths for files and urls for everything else is fine internally but the 
> API was not very clear about how to use it.
> Given you would need to basically parse the resource to know which one it is, 
> if you did not forget to do it in the first place.
> That's why D22005  happens.
> 
> IMO we would need a type dedicated for file path, that would be a wrapper 
> around QString, something like C++17 
> https://en.cppreference.com/w/cpp/filesystem/path or Rust 
> https://doc.rust-lang.org/std/path/struct.Path.html
> 
> While we are at it I could add a isPath() or similar to tell if resource 
> contains a url or a path QDir::isAbsolutePath(resource()) basically.
> 
> About KF6 I would suggest resource would return something like 
> std::variant 
> https://en.cppreference.com/w/cpp/utility/variant
> Add a std::optionnal path() and make url std::optionnal could 
> also be interesting.
> Can't wait for KF6 C++17 !
> I learned a lot of those modern C++ features first in Rust.

Ok, agreed. The reason why I thought the `resourceUrl` is a better choice is 
that it is an url of the resource, not of the result. But I agree `url` is 
cleaner.

We'll see about the KF6 part. variants/optionals vs a proxy type that converts 
to QString and QUrl in a correct way :)

REPOSITORY
  R159 KActivities Statistics

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D24741

To: meven, ivan
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24741: Add a utility accessor to get a QUrl from a ResultSet::Result

[Ivan Čukić]

ivan requested changes to this revision.
ivan added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> resultset.h:78
> +QString resource() const; ///< String representation of 
> resource (can represent an url or a path)
> +QUrl url() const; ///< Url representation of a 
> resource based on internal resource, readonly, @since 5.64
>  QString title() const;///< Title of the resource, or 
> URL if title is not known

`url` or `resourceUrl`?

I hoped we are not going to have these problems after the death of Nepomuk. 
Thought file paths for files and urls for everything else would be a sane 
default. :)

Also, can you add a `TODO: KF6 rething the function names` for these two.

REPOSITORY
  R159 KActivities Statistics

REVISION DETAIL
  https://phabricator.kde.org/D24741

To: meven, ivan
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

Re: Framework for encrypted storage for apps

[Ivan Čukić]

Hi Johan, 

One important question here is how do you want the secrets to be unlocked.
Is the user going to type in some master password every time the otpclient is 
started?

If that is the case, you can use QCA to encrypt and decrypt the data you need
to store securely.

If that is not the case - if you don't want to always ask the user for the 
password, then you can expect quite a complicated story of how to make 
something like kwallet safe.

>  1. (On KDE) these libraries wind up calling dbus. We would like to
> avoid plain text out-of-process communication of secrets, especially
> communication through a 'shared' channel (for obvious reasons).
>  2. In particular with KDE Wallet it was suggested that the wallet
> itself may, in fact, not be encrypted.

Another huge problem of kwallet is that it can not authenticate which 
application is asking for the password. I was told that this could be easier 
to do with flatpaks, but it will still need significant changes to how kwallet 
works.

> Another possible contender is Plasma Vault, but we don't really know
> how that would work in the context of an app, especially a flatpak
> app.

An approach like the one taken in Plasma Vault could work for applications,
but it would (like the QCA option previously mentioned) require the user to
type in the password every time. The problem with using Plasma Vault is
that it works with fuse mounts - so the decrypted data si visible through the 
filesystem. (though, flatpaks could maybe have some solution there as well, 
not my area)

Cheers,
Ivan


-- 
dr Ivan Čukić
KDE, ivan.cu...@kde.org, https://cukic.co/
gpg key fingerprint: 8FE4 D32F 7061 EA9C 8232  07AE 01C6 CE2B FF04 1C12

D23742: Add two special cases url recentlyused:/files and recentlyused:/folders

[Ivan Čukić]

ivan accepted this revision.

REPOSITORY
  R320 KIO Extras

BRANCH
  arcpatch-D23742

REVISION DETAIL
  https://phabricator.kde.org/D23742

To: meven, ivan, ngraham, #frameworks
Cc: kde-frameworks-devel, kfm-devel, iasensio, fprice, LeGast00n, MrPepe, 
fbampaloukas, alexde, GB_2, Codezela, feverfew, meven, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov

D23736: Add Term::Type::files() and Term::Type::directories() to filter only directories or excluding them

[Ivan Čukić]

ivan accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R159 KActivities Statistics

BRANCH
  arcpatch-D23736

REVISION DETAIL
  https://phabricator.kde.org/D23736

To: meven, ivan
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D23736: Add Term::Type::files() and Term::Type::locations() to filter excluding or only directories

[Ivan Čukić]

ivan added inline comments.

INLINE COMMENTS

> terms.h:106
> + */
> +static Type locations();
>  

Can we call it `directories` to follow the Qt API? That is the only complaint I 
have, otherwise thie is quite a nice addition.

REPOSITORY
  R159 KActivities Statistics

REVISION DETAIL
  https://phabricator.kde.org/D23736

To: meven, ivan
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23742: Add two special cases url recentlyused:/files and recentlyused:/folders

[Ivan Čukić]

ivan accepted this revision.

REPOSITORY
  R320 KIO Extras

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D23742

To: meven, ivan, ngraham, #frameworks
Cc: kde-frameworks-devel, kfm-devel, iasensio, fprice, LeGast00n, MrPepe, 
fbampaloukas, alexde, GB_2, Codezela, feverfew, meven, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov

D23736: Add a Term::Type::files() to filter excluding directories

[Ivan Čukić]

ivan requested changes to this revision.
ivan added a comment.
This revision now requires changes to proceed.


  Add the same for directories for completeness

REPOSITORY
  R159 KActivities Statistics

REVISION DETAIL
  https://phabricator.kde.org/D23736

To: meven, ivan
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23555: Add @since 5.62 for newly added setters

[Ivan Čukić]

ivan accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R159 KActivities Statistics

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D23555

To: meven, ivan, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23556: Add version requirement to dependency KActivitiesStats

[Ivan Čukić]

ivan accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R320 KIO Extras

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D23556

To: meven, #frameworks, kossebau, ivan
Cc: kde-frameworks-devel, kfm-devel, vmarinescu, fprice, LeGast00n, MrPepe, 
fbampaloukas, alexde, GB_2, Codezela, feverfew, meven, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov

D22144: Add kio recentlyused:/ to access KActivityStats data

[Ivan Čukić]

ivan accepted this revision.
ivan added a comment.
This revision is now accepted and ready to land.


  Filtering can be done as far as the KAStats is concerned. The question is how 
to know inside the KIO slave which application requested its services.

REPOSITORY
  R320 KIO Extras

BRANCH
  arcpatch-D22144_1

REVISION DETAIL
  https://phabricator.kde.org/D22144

To: meven, ivan, #frameworks, ngraham, dfaure
Cc: dhaumann, elvisangelaccio, kde-frameworks-devel, kfm-devel, aprcela, 
vmarinescu, fprice, LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, Codezela, 
feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, mikesomov

D22143: Add proper logging using ECMQtDeclareLoggingCategory

[Ivan Čukić]

ivan accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R159 KActivities Statistics

BRANCH
  arcpatch-D22143

REVISION DETAIL
  https://phabricator.kde.org/D22143

To: meven, ivan, #frameworks, kossebau
Cc: kossebau, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D22143: Add proper logging using ECMQtDeclareLoggingCategory

[Ivan Čukić]

ivan added inline comments.

INLINE COMMENTS

> kactivities-stats.categories:1
> +org.kde.kactivities.stats KActivities Stats DEFAULT_SEVERITY [WARNING] 
> IDENTIFIER [KACTIVITY_STAT_LOG]

Missed one here

> CMakeLists.txt:21
> +HEADER kactivities-stats-logsettings.h
> +IDENTIFIER KACTIVITIE_STATS_LOG
> +CATEGORY_NAME kf5.kactivity.stat)

KACTIVITIE -> KACTIVITIE**S **

REPOSITORY
  R159 KActivities Statistics

REVISION DETAIL
  https://phabricator.kde.org/D22143

To: meven, ivan, #frameworks, kossebau
Cc: kossebau, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D22143: Add proper logging using ECMQtDeclareLoggingCategory

[Ivan Čukić]

ivan requested changes to this revision.
ivan added a comment.
This revision now requires changes to proceed.


  Change everything to plural.
  
  I agree with @kossebau, though I don't think it will lead to problems in 
future, so I could say it is bearable dirtyness.

INLINE COMMENTS

> CMakeLists.txt:22
> +   # Generated by macro ecm_qt_declare_logging_category in src/CMakeLists.txt
> +   ${CMAKE_BINARY_DIR}/src/kactivities-stat-logsettings.cpp
> +

plural `stats`

> kactivities-stats.categories:1
> +org.kde.kactivities.stats KActivities Stats DEFAULT_SEVERITY [WARNING] 
> IDENTIFIER [KACTIVITY_STAT_LOG]

You mixed singulars and plurals - go for plurals everywhere:

`s/KACTIVITY_STAT_LOG/KACTIVITIES_STATS_LOG/g`

REPOSITORY
  R159 KActivities Statistics

REVISION DETAIL
  https://phabricator.kde.org/D22143

To: meven, ivan, #frameworks, kossebau
Cc: kossebau, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23399: Add setter to Type, Activity, Agent and UrlFilter query fields

[Ivan Čukić]

ivan accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R159 KActivities Statistics

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D23399

To: meven, ivan
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23399: Add setter to Type, Activity, Agent and UrlFilter query fields

[Ivan Čukić]

ivan added inline comments.

INLINE COMMENTS

> query.h:105
> +void setUrlFilters(const QStringList &urlFilters);
> +void setType(const Terms::Type &types);
> +void setAgent(const Terms::Agent &agents);

I screwed up the names - plurals vs singulars here as `Agent` can contain 
several agents - having `Agent::current` looked better than `Agents::current`.

We'll need to do something about this for Qt6/KF6.

Now, I can not guarantee this, but I think we will have a smaller API change if 
these were all named in plural. In that case, with the implicit conversion from 
`QStringList` to `Agent` and others, only one `set`  function should be needed 
for each of these.

So

  void setTypes(const Terms::Type &types);

instead of

  void setTypes(const QStringList &types);
  void setType(const Terms::Type &types);

What do you think?

REPOSITORY
  R159 KActivities Statistics

REVISION DETAIL
  https://phabricator.kde.org/D23399

To: meven, ivan
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23393: Use special values constants in terms.cpp

[Ivan Čukić]

ivan accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R159 KActivities Statistics

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D23393

To: meven, ivan
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D22144: Add kio recentlyused:/ to access KActivityStats data

[Ivan Čukić]

ivan added inline comments.

INLINE COMMENTS

> meven wrote in recentlyused.cpp:109
> We don't have yet set function for Activity, Type, Agent, and Url only add.
> Which means we would need to use query.addActivity(QStringList() << ":any")) 
> sometimes for instance, that is not ideal since it makes the user class use 
> internal details such as ":any".
> I would be in favor to add setActivity(Terms::Activity) in KactivitiesStats 
> same for Type, Agent and Url, to get arount that and maybe also 
> addActivity(Terms::Activity) as well for consistency.

You have `Activity::any()` - no need for the client to use the special values. 
You can do a `addActivities({Activity::any()})`.

I would not mind to add `set*` variants of `add*` functions - `setActivities`, 
`setTypes`, etc. to avoid the need of calling `clear*` and then `add*`.

I'm also thinking whether it would be useful to have a variadic template 
versions of these so that you can skip the `QList` construction. That might 
be better to wait for Qt 6 and raised C++ version requirement. Not sure.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D22144

To: meven, ivan, #frameworks, ngraham, dfaure
Cc: dhaumann, elvisangelaccio, kde-frameworks-devel, kfm-devel, aprcela, 
vmarinescu, fprice, LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, Codezela, 
feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, mikesomov

D22144: Add kio recentlyused:/ to access KActivityStats data

[Ivan Čukić]

ivan added inline comments.

INLINE COMMENTS

> meven wrote in recentlyused.cpp:109
> Tried implementing this in D23372  but 
> @ivan was not found of it because of induced complexity it entails to work 
> properly.

Also, there are `set*` and `add*` member functions which can be used for 
non-chained changes.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D22144

To: meven, ivan, #frameworks, ngraham, dfaure
Cc: dhaumann, elvisangelaccio, kde-frameworks-devel, kfm-devel, aprcela, 
vmarinescu, fprice, LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, Codezela, 
feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, mikesomov

D23372: Add operator |= for Query Terms

[Ivan Čukić]

ivan added a comment.


  You can also use `set*/add*` member functions for this so that David doesn't 
complain. As I said, pipes are mainly there for chaining - easy query creation 
API to simplify the alternative of set set set set set. But if you set a single 
field, I'd say `set*/add*` is a fine option.

REPOSITORY
  R159 KActivities Statistics

REVISION DETAIL
  https://phabricator.kde.org/D23372

To: meven, ivan
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23372: Add operator |= for Query Terms

[Ivan Čukić]

ivan requested changes to this revision.
ivan added a comment.
This revision now requires changes to proceed.


  This is something I did have initally in kastats, it was killed because while 
this is tempting
  
query |= Type(types);
  
  it ruins the idea of the chaning API because it can not be used like this:
  
query |= Type(types) | OtherTerm(...);
  
  In order for this to work properly, we'd need to use expression templates 
which would make the library significanly more complex.
  
  I don't think it is worth it.

REPOSITORY
  R159 KActivities Statistics

REVISION DETAIL
  https://phabricator.kde.org/D23372

To: meven, ivan
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D22775: Allow date range filtering of resource events using Date Term

[Ivan Čukić]

ivan accepted this revision.
ivan added a comment.
This revision is now accepted and ready to land.


  Sorry I missed this one. If I don't react on a ping here, just send me a 
direct mail.

REPOSITORY
  R159 KActivities Statistics

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D22775

To: meven, ivan, #frameworks
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D22144: Add kio recentlyused:/ to access KActivityStats data

[Ivan Čukić]

ivan added a comment.


  Looks OK to me, I guess some of our resident KIO experts should review it. 
What do you think?

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D22144

To: meven, ivan, #frameworks, ngraham
Cc: elvisangelaccio, kde-frameworks-devel, kfm-devel, aprcela, vmarinescu, 
fprice, LeGast00n, MrPepe, fbampaloukas, alexde, Codezela, feverfew, meven, 
michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, mikesomov

D23112: Add a event Spy for GtkFileChooser recent files

[Ivan Čukić]

ivan accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R161 KActivity Manager Service

BRANCH
  arcpatch-D23112

REVISION DETAIL
  https://phabricator.kde.org/D23112

To: meven, #frameworks, ivan
Cc: bruns, ngraham, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D23112: Add a event Spy for GtkFileChooser recent files

[Ivan Čukić]

ivan requested changes to this revision.
ivan added a comment.
This revision now requires changes to proceed.


  Another tiny change, and I think it is ready to land. Unless someone else 
sees other issues.

INLINE COMMENTS

> GtkEventSpy.cpp:126
> +
> +if (exec.at(0) == QStringLiteral("'") && exec.at(exec.size() - 1) == 
> QStringLiteral("'")) {
> +// remove "'" caracters wrapping the command

No need to compare chars with strings:

  if (!exec.isEmpty() && exec[0] == '\'' && exec.back() == '\'')

REPOSITORY
  R161 KActivity Manager Service

REVISION DETAIL
  https://phabricator.kde.org/D23112

To: meven, #frameworks, ivan
Cc: ngraham, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D23112: Add a event Spy for GtkFileChooser recent files

[Ivan Čukić]

ivan added inline comments.

INLINE COMMENTS

> GtkEventSpy.cpp:68
> +QDateTime visited;
> +QList *applications;
> +

No need for this to be a pointer to a list. Make it just `QList 
applications`.

> GtkEventSpy.cpp:70
> +
> +Bookmark(){
> +applications = new QList();

The constructor will not be needed once `applications` stops being a pointer.

> GtkEventSpy.cpp:76
> +
> +QString Bookmark::latestApplication() const {
> +Application current = applications->first();

`{` which starts a function should be on a new line (I don't care much about 
this, but let's follow the KF5 style)

> GtkEventSpy.cpp:77-78
> +QString Bookmark::latestApplication() const {
> +Application current = applications->first();
> +for (const Application &app : qAsConst(*applications)) {
> +if (app.modified > current.modified) {

When you make `applications` not to be a pointer `qAsConst` will not be needed 
as this is a `const` member function.

> GtkEventSpy.cpp:89
> +public:
> +BookmarkHandler(){
> +current = nullptr;

Replace with:

  BookmarkHandler()
  : current(nullptr)
  {
  }

> GtkEventSpy.cpp:91
> +current = nullptr;
> +marks = QList();
> +}

No need for this - marks are already default-constructed.

> GtkEventSpy.cpp:113
> +if (qName == QStringLiteral("bookmark")) {
> +current = new Bookmark();
> +current->href = QUrl(attributes.value("href"));

No need for dynamic allocation. Make it a normal variable instead of a pointer.

> GtkEventSpy.cpp:185
> +reader.setErrorHandler(&bookmarkHandler);
> +QXmlInputSource *source = new QXmlInputSource(&file);
> +

No need for dynamic allocation. Make it a normal variable instead of a pointer.

> meven wrote in GtkEventSpy.cpp:143
> It is to just extract the executable name, we don't want to have an exploding 
> number of initiatingAgent for every argument and parameter that might come 
> through here.

I meant it will be a problem if someone decides to have a space in the 
executable like `my\ aweomse\ binary`. But this should not be an issue at the 
moment.

REPOSITORY
  R161 KActivity Manager Service

REVISION DETAIL
  https://phabricator.kde.org/D23112

To: meven, #frameworks, ivan
Cc: ngraham, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D23112: Add a event Spy for GtkFileChooser recent files

[Ivan Čukić]

ivan requested changes to this revision.
ivan added a comment.
This revision now requires changes to proceed.


  Thanks for geting involved this much!

INLINE COMMENTS

> GtkEventSpy.cpp:129
> +// remove "'" caracters wrapping the command
> +exec = exec.mid(1, exec.size() -2 );
> +

It might be dangerous to assume that the command is wrapped with single quotes

> GtkEventSpy.cpp:133
> +// See 
> https://techbase.kde.org/Development/Tutorials/Services/Traders#The_KTrader_Query_Language
> +QString query = QString("exist Exec and ((Exec ~~ '%1') or (exist 
> GenericName and '%1' ~~ GenericName) or (exist Name and '%1' ~~ 
> Name))").arg(exec);
> +KService::List services = 
> KServiceTypeTrader::self()->query(QStringLiteral("Application"), query);

const auto query

> GtkEventSpy.cpp:134
> +QString query = QString("exist Exec and ((Exec ~~ '%1') or (exist 
> GenericName and '%1' ~~ GenericName) or (exist Name and '%1' ~~ 
> Name))").arg(exec);
> +KService::List services = 
> KServiceTypeTrader::self()->query(QStringLiteral("Application"), query);
> +

This should also be `const`, other local variables as well

> GtkEventSpy.cpp:143
> +// remove space and any caracter after
> +int spaceIdex = exec.indexOf(" ");
> +if (spaceIdex != -1) {

Let's hope commands will never have spaces in them :)

REPOSITORY
  R161 KActivity Manager Service

REVISION DETAIL
  https://phabricator.kde.org/D23112

To: meven, #frameworks, ivan
Cc: broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, 
GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D22717: Add Date term to KActivities Stats to filter on resource event date

[Ivan Čukić]

ivan accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R159 KActivities Statistics

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D22717

To: meven, ivan, #frameworks
Cc: kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, bruns

D22143: Add proper logging using ECMQtDeclareLoggingCategory

[Ivan Čukić]

ivan added a reviewer: kossebau.

REPOSITORY
  R159 KActivities Statistics

REVISION DETAIL
  https://phabricator.kde.org/D22143

To: meven, ivan, #frameworks, kossebau
Cc: kossebau, kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, 
bruns

D22143: Add proper logging using ECMQtDeclareLoggingCategory

[Ivan Čukić]

ivan added a comment.


  If @kossebau is satisfied, go for it

REPOSITORY
  R159 KActivities Statistics

REVISION DETAIL
  https://phabricator.kde.org/D22143

To: meven, ivan, #frameworks
Cc: kossebau, kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, 
bruns

D22717: Add Date term to KActivities Stats to filter on resource event date

[Ivan Čukić]

ivan added inline comments.

INLINE COMMENTS

> resultset.cpp:293
>  SELECT
> -rl.targettedResource as resource
> -  , SUM(rsc.cachedScore) as score
> -  , MIN(rsc.firstUpdate) as firstUpdate
> -  , MAX(rsc.lastUpdate)  as lastUpdate
> -  , rl.usedActivity  as activity
> -  , rl.initiatingAgent   as agent
> -  , COALESCE(ri.title, rl.targettedResource) as title
> +from_table.targettedResource as resource
> +  , SUM(rsc.cachedScore) as score

Any reason for the rename?

REPOSITORY
  R159 KActivities Statistics

REVISION DETAIL
  https://phabricator.kde.org/D22717

To: meven, ivan, #frameworks
Cc: kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, bruns

D22717: Add Date term to KActivities Stats to filter on resource event date

[Ivan Čukić]

ivan requested changes to this revision.
ivan added a comment.
This revision now requires changes to proceed.


  Joining with `ResourceEvent` is a problem. At least make it not get joined if 
the date filter is not present.

INLINE COMMENTS

> QueryTest.h:53
>  void testFancySyntaxOrderingDefinition();
> +void tesDateSyntaxOrderingDefinition();
>  

tes -> test

> resultset.cpp:182
>  
> +QString dateClause(const QDate &date) const {
> +return date.isNull() ? QStringLiteral("1") :

setDate above is by value, here it is const-ref. Check for the size of the type 
and decide on one of these.

> resultset.cpp:298
>  ON rl.targettedResource = ri.targettedResource
> +   LEFT JOIN
> +   ResourceEvent re

This is a potential efficiency problem - ResourceEvent is the quickest growing 
table. I don't like the idea for it to always be joined.

> resultset.cpp:416
>  ResourceScoreCache rsc
> +LEFT JOIN
> +   ResourceEvent re

Also, add an empty line above - I know I like empty lines too much, but ... :)

REPOSITORY
  R159 KActivities Statistics

REVISION DETAIL
  https://phabricator.kde.org/D22717

To: meven, ivan, #frameworks
Cc: kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, bruns

D22144: Add kio recentlyused:/ to access KActivityStats data

[Ivan Čukić]

ivan requested changes to this revision.
ivan added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> recentlyused.cpp:140-144
> +if (agentValue.contains(QLatin1Char(','))) {
> +query = query | Agent(agentValue.split(QLatin1Char(',')));
> +} else {
> +query = query | Agent(agentValue);
> +}

Could this just be replaced with `query = query | 
Agent(agentValue.split(QLatin1Char(',')));`?

Obviously, the downside is always creating a list (usually with a single item 
in it), the upside is that it does not need to traverse the string twice (once 
to chech for commas, and the second time to split the string.

The trade-off depends on internals of Qt - if  you don't have the time to 
benchmark what is faster, leave it as is.

> recentlyused.cpp:250
> +if (isRootUrl(url)) {
> +mimeType(QString::fromLatin1("inode/directory"));
> +finished();

`QStringLiteral`?

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D22144

To: meven, ivan, #frameworks, ngraham
Cc: elvisangelaccio, kde-frameworks-devel, kfm-devel, aprcela, fprice, 
LeGast00n, sbergeron, fbampaloukas, alexde, feverfew, meven, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov

D22082: WIP Add an ioslave to access KActivityStat data

[Ivan Čukić]

ivan added a comment.


  I'd add support for `HighScoredFirst` and `RecentlyCreatedFirst`. Maybe the 
protocol could be something like "used:/" or something.
  
  It should also support getting items for the current activity, or all 
recently used items.

REPOSITORY
  R159 KActivities Statistics

REVISION DETAIL
  https://phabricator.kde.org/D22082

To: meven, #frameworks, ngraham, ivan
Cc: ivan, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D22091: Fix a crash in KactivityTestApp when Result has strings with non-ASCII

[Ivan Čukić]

ivan accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R159 KActivities Statistics

BRANCH
  arcpatch-D22091

REVISION DETAIL
  https://phabricator.kde.org/D22091

To: meven, ivan, broulik, #plasma, #frameworks
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D20938: Add Mounts Backend

[Ivan Čukić]

ivan added a comment.


  WRT implementation, I'd also add a link to KMountPoint 
(https://api.kde.org/frameworks/kio/html/classKMountPoint.html) which provides 
an interface to fstab/mtab so that Solid/FuseMounts plugin does not need to do 
any parsing by itself.
  
  It works with 'fuse.*'

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D20938

To: hallas, #frameworks, ngraham, elvisangelaccio, broulik, bruns
Cc: svuorela, nicolasfella, ivan, kde-frameworks-devel, michaelh, ngraham, bruns

D20938: Add Mounts Backend

[Ivan Čukić]

ivan added a comment.


  @ngraham
  
  I guess that **is** the main point of this patch. I don't think that anyone 
is against that. :)

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D20938

To: hallas, #frameworks, ngraham, elvisangelaccio, broulik, bruns
Cc: nicolasfella, ivan, kde-frameworks-devel, michaelh, ngraham, bruns

D20938: Add Mounts Backend

[Ivan Čukić]

ivan added a comment.


  I'm torn between two approaches:
  
  - doing what you have done, maybe with a customization point - `fusermount 
-u` by default, something else for specific mount types;
  - disabling the teardown operation
  
  The rationale for the second one:
  
  - The users which create fuse mounts from the shell know how to unmount them;
  - Users which used a tool to mount something (like Plasma Vault) should use 
the same tool to unmount (these tools potentially do more than simple 
unmounting - like calling a destructor in C++ instead of just free :) );
  - Some applications will have their own mounts (for example, akonadi will use 
encrypted storage for storing indexes of encrypted mails) - you don't want to 
have those controlled by the user.*
  
  (*) we will also need to hide these from Places, but that is not important at 
this point.

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D20938

To: hallas, #frameworks, ngraham, elvisangelaccio, broulik, bruns
Cc: nicolasfella, ivan, kde-frameworks-devel, michaelh, ngraham, bruns

D20938: Add Mounts Backend

[Ivan Čukić]

ivan added a comment.


  Thanks for working on this.
  
  I'd probably call this `fusemounts` backend as it handles only FUSE mounts 
and nothing else.
  
  As for the //teardown// operation, some FUSE backends (cryfs >0.10) have 
their own unmount commands instead of `fusermount -u`.

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D20938

To: hallas, #frameworks, ngraham, elvisangelaccio, broulik, bruns
Cc: ivan, kde-frameworks-devel, michaelh, ngraham, bruns

D19979: Don't create thumbnails for encrypted Vaults

[Ivan Čukić]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:bc42a1b2f913: Don't create thumbnails for encrypted 
Vaults (authored by ivan).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19979?vs=54673&id=54678

REVISION DETAIL
  https://phabricator.kde.org/D19979

AFFECTED FILES
  src/widgets/previewjob.cpp

To: ivan, davidedmundson, dfaure
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns

D19979: Don't create thumbnails for encrypted Vaults

[Ivan Čukić]

ivan updated this revision to Diff 54673.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19979?vs=54582&id=54673

REVISION DETAIL
  https://phabricator.kde.org/D19979

AFFECTED FILES
  src/widgets/previewjob.cpp

To: ivan, davidedmundson, dfaure
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns

D19979: Don't create thumbnails for encrypted Vaults

[Ivan Čukić]

ivan marked an inline comment as done.
ivan added inline comments.

INLINE COMMENTS

> broulik wrote in previewjob.cpp:307
> Did you profile the impact of this call? I don't think it's cached. Can this 
> maybe be moved to the thumbnail KIO so it's done out of process?

I didn't do any profiling.

We can not cache because mount points can change. KMountPoint provides no 
signals of this.

It should not be overly expensive - small number of mounts to process.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D19979

To: ivan, davidedmundson, dfaure
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns

D19986: Install .desktop file for kded5

[Ivan Čukić]

ivan accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R297 KDED

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D19986

To: davidedmundson, ivan
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D19979: Don't create thumbnails for encrypted Vaults

[Ivan Čukić]

ivan edited the summary of this revision.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D19979

To: ivan, davidedmundson, dfaure
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D19979: Don't create thumbnails for encrypted Vaults

[Ivan Čukić]

ivan retitled this revision from "Don't create thumbnails for encrypted drives 
(Vaults)" to "Don't create thumbnails for encrypted Vaults".

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D19979

To: ivan, davidedmundson, dfaure
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D19979: Don't create thumbnails for encrypted drives (Vaults)

[Ivan Čukić]

ivan updated this revision to Diff 54582.
ivan added a comment.


  Incorporated David's advice

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19979?vs=54577&id=54582

REVISION DETAIL
  https://phabricator.kde.org/D19979

AFFECTED FILES
  src/widgets/previewjob.cpp

To: ivan, davidedmundson, dfaure
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D19979: Don't create thumbnails for encrypted drives (Vaults)

[Ivan Čukić]

ivan created this revision.
ivan added reviewers: davidedmundson, dfaure.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
ivan requested review of this revision.

REVISION SUMMARY
  This patch makes PreviewJob skip thumbnail generation on fuse.encfs and 
fuse.cryfs mounts.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D19979

AFFECTED FILES
  src/widgets/previewjob.cpp

To: ivan, davidedmundson, dfaure
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D16250: Use lambdas instead of std::bind()

[Ivan Čukić]

ivan added a comment.


  @davidedmundson
  
  Lambdas are generally faster to compile and execute. `std::bind` can make 
code less verbose, but that is not the case here.
  
  @bruns
  
  Well... Qt has quite a few strange guidelines that we don't follow :)

REPOSITORY
  R127 KWayland

REVISION DETAIL
  https://phabricator.kde.org/D16250

To: volkov, #kwin
Cc: bruns, davidedmundson, ivan, kde-frameworks-devel, michaelh, ngraham

D16250: Use lambdas instead of std::bind()

[Ivan Čukić]

ivan added a comment.


  The parentheses are not necessary when a lambda has no arguments.
  
[something]() { ::: }
  
  can become:
  
[something] { ::: }
  
  KWin uses both of these from what I've seen. Personally, I prefer the second.
  
  Otherwise, +1 for the change.

REPOSITORY
  R127 KWayland

REVISION DETAIL
  https://phabricator.kde.org/D16250

To: volkov, #kwin
Cc: ivan, kde-frameworks-devel, michaelh, ngraham, bruns

Re: Password field security and information leaking

[Ivan Čukić]

 
> It seems to me that a notional end-user application is the one that
> actually needs to *use* the password though, which might imply that the
> U/I for entering the password needs to be in a different process?

Depends.

I investigated all of this primarily for Plasma Vaults. The implementation is 
split into a Plasma applet and a kded module. Currently, the password 
requesting UI is in the kded module because I didn't want for it to go between 
processes in an insecure manner.

With the proposed change, the password entry could go into the Plasma Applet, 
where the password user would be the kded module.

I think the same organization would be fitting for some other applications as 
well. Consider KMail - password entry is in KMail, while the Akonadi agents 
are the users.

> Or might it instead be possible to split things up even more, so that
> end-user applications speak to an authentication agent, which performs
> authentication on the application's behalf, and the agent itself handles
> U/I separately? Again, similar to existing GPG and SSH model.
> 
> The downside I see is that it might be confusing and distracting for
> users to do 99% of their application configuration inside the
> application itself, and then have to go through an "auth agent popup
> window" experience for the authentication piece only.
> 
> But even there, if the application is able to use a secure password
> entry widget, it could perhaps be possible to be able to safely set or
> update authentication credentials in the same app config U/I (i.e. have
> the app itself serve the pinentry-qt role).

This sounds interesting. Let's see how this evolves. I'll use Vaults as a 
guinea pig, and I'll try to make it generic enough to support both single-
process entry+usage and the previously mentioned split.

I agree that it would be awkward to have the password entry outside of an 
application. If it's in the same process as the password user, I'm not sure 
doing a roundabout through an agent would have any benefits. Unless the agent 
also serves for 'remembering' the password like some KWallet++ (the formerly 
planned KSecretService). 

> GPG and SSH already have some facilities for handling this use case, I
> think. Would it be possible to e.g. use pinentry-qt for this?

Pinentry communication is not encrypted. But, we could provide a nice 
pinentry-kde if we wanted that supports the pinentry protocol and adds our own 
extensions for encrypted communication.

Cheers,
Ivan




dr Ivan Čukić
KDE, ivan.cu...@kde.org, https://cukic.co/
gpg key fingerprint: 8FE4 D32F 7061 EA9C 8232  07AE 01C6 CE2B FF04 1C12

Re: Password field security and information leaking

[Ivan Čukić]

> Is this a problem at all?
> I mean, by default applications can't read other applications memory so the
> only one that can try this kind of attacks is the root user.

Yes it is a problem.

It would be like saying no need to encrypt the hard drive - the privilege 
system is enough.

Some of the recent side-channel attacks (like Spectre) allow partial memory 
dumps. Mostly in-process memory dumps, but this is just the beginning of side-
channel exploits. You can expect that these will evolve over the years in a 
similar fashion to buffer overflow attacks of yesteryear.

Also the potential of writing said data to the swap is another problem. Also 
cold-boot attacks.

And, if we want to be the software collection that needs to be a privacy 
heaven (see Privacy Goal https://phabricator.kde.org/T7050), we need to 
provide utilities as safe as possible even for people like Snowden.


> If your system is compromised to the fact that root is evil you have lost
> already, surely root can install a key logger or something that will make
> it easy for her to snoop your passwords than having the grep the memory for
> them, no? (Well on X11 any application can install a keylogger but let's
> assume you're under Wayland :D)

Having easier attack vectors (which are not always available - for example, in 
the cold-boot scenario etc.) does not give us the excuse to provide more 
attack vectors.


> AFAIR there's private dbus through apparmor, that'd be a simpler fix,
> unfortunately apparmor is not default on all distros so doesn't really
> solve much for us.



> Not sure if any other dbus or kernel security enhacements implement or plan
> to implement this.

As mentioned in the previous mail, the similar approach is used by Secret 
Service.

> but who cares about that? At this point i can just go to kwalletd and write
> a program that will say "Hey, I'm the network manager, give me this
> password" and there's no way for kwalletd to figure out if this is true or
> not, so it'll just give that program access to all my passwords without any
> need to snoop at the transport layer.

That is a completely separate issue. KWallet design is broken.


> I've been thinking about that for a while and the only solution i've found
> is signing the binaries.

I agree. It would be nice if someone wanted to work on that as well.

Cheers,
Ivan




dr Ivan Čukić
KDE, ivan.cu...@kde.org, https://cukic.co/
gpg key fingerprint: 8FE4 D32F 7061 EA9C 8232  07AE 01C6 CE2B FF04 1C12

Password field security and information leaking

[Ivan Čukić]

Hi all,


We are using QLineEdit (and QML equivalent) all over KDE for passwords. This 
is an issue for types of attacks that extract raw process memory because the 
passwords can be leaked.

QLineEdit stores the current value as plain text in a QString. Which means 
that while the password entry is shown in the screen, the password is stored 
in memory for all to see (problem 1).

When the QLineEdit is destroyed, so is the QString. But, while the data buffer 
is 'deleted', its contents remain in memory until some other dynamically 
allocated object is created in the same memory space and overwrites the data - 
this is because QString does not zero-out its buffer on destruction. This 
means that the passwords remain in memory for much longer than needed (problem 
2).

Because of string reallocation on resizing, it is quite possible that, while 
the user is typing the password, that the string will be resized/reallocated 
at least once. This means that partial passwords (data from the buffer in the 
string before reallocation which is not zeroed-out) will remain in memory 
until something else overwrites them (problem 3).

All this memory can end up written to the hdd/ssd if the application is moved 
to swap by the OS (problem 4).

Transporting passwords via DBus (KWallet) is the problem 5 (guessing there's 
no need to explain this one in more detail).


Potential solutions:

Problem 2 is easily solved in Qt by having QLineEdit zero-out the string data 
on destruction if the line edit is used as a password entry. I've submitted a 
patch for this [1]. Another additional change that can be applied to QLineEdit 
is to call QString::reserve(50-or-something) when the line edit is used as a 
password field to minimize the possibility of problem 3 occurring for most 
use-cases.

Other problems are more difficult to solve. They need a custom component (from 
what I can tell).

Namely, the least a proper password entry component could do is to use a 
custom QString-like class which zeroes-out buffers before deleting them (on 
reallocation and destruction), and which tells the OS that the value contents 
should never be swapped. There is a class in QCA that can be used for this 
(SecureArray, based on botan::SecureVector).

Having a password entry component that uses a secure buffer like SecureArray 
would minimize the time the entered password is in memory to only while the 
component is shown (problem 1).

The only remaining problems are 1 and 5.

Problem 1 can not be completely solved. If the password data is kept in memory 
encrypted, the key for decryption needs to also be somewhere so that the 
program can get the actual password instead of the encrypted version.

Still, it would be an improvement over the current situation because the 
encryption password would be some random array of bytes, and the encrypted 
passwords would appear as random arrays of bytes (when encryption is good, 
encrypted data looks random). Which means that any attacks with the aim of 
extracting the password data couldn't be based on text-processing - they would 
need quite some effort of analyzing memory layout to find both the key and the 
password in order to decrypt it.


Now comes the most interesting part (IMO). which solves problem 5 and also 
influences the previous issue of having the decryption key in memory.

We can use a stream cipher to encrypt the password in memory. This means that 
the password characters can be encrypted one by one as the user types them in 
- no need to have the whole password unencrypted in memory at any point in 
time during password entry.

If we use public/private key encryption, where the password entry is in one 
process, and the password usage is in another, only the password usage process 
will be able to decrypt the password (and, again, there will be no need to the 
whole password to be decrypted at once - just one character at a time can be 
sufficient in many use-cases). This means that the attacker will need to have 
either access to the memory spaces of both processes, or to hit a very narrow 
window in the 'usage process' between 'I got the password' and 'I used the 
password and zeroed-out the data'.

The pub/priv key exchange can be done mostly securely using Diffie-Hellman 
protocol similar to what the Secret Service API [2] does. The only attack 
vector would be to replace the normal dbus server with a malicious one. In 
that case (having a malicious system component), I'd say the systems is 
already compromised and that we can not do anything to protect the user.


Thoughts?


Cheers,
Ivan

[1] https://codereview.qt-project.org/#/c/242202/
[2] https://specifications.freedesktop.org/secret-service/index.html



dr Ivan Čukić
KDE, ivan.cu...@kde.org, https://cukic.co/
gpg key fingerprint: 8FE4 D32F 7061 EA9C 8232  07AE 01C6 CE2B FF04 1C12

D13980: Honor BUILD_TESTING

[Ivan Čukić]

ivan accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R6 KActivities

REVISION DETAIL
  https://phabricator.kde.org/D13980

To: arojas, ivan
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

Re: bic in kactivities

[Ivan Čukić]

Marco, thanks for reverting this.

@Laurent

The proper change for the constructor should be to make the Private class
take the shownStates by value (a sink constructor argument) and for the
ActivitiesModel constructor to std::move it when passing to the Private
constructor.

@All

- ` explicit` should not change the ABI (will have to check just in case)
- The second change that shouldn't be a problem is that
src/imports/resourcemodel.h does not need to keep ABI  - it not exported as
a normal library, it is a QML type.

This does not need the FIXME KF6 tag, the API is as it should be,

Cheers,
Ivan

Re: [kcoreaddons] src/lib: Revert "Warning--"

[Ivan Čukić]

Sorry for the noise, saw that the patch was fixed and committed again. Gah...

Re: [kcoreaddons] src/lib: Revert "Warning--"

[Ivan Čukić]

Hi Ben,

Is it possible to have cgit.kde.org show commit hashes in the 'log'
tab (for example [1])?

As for the Laurent's patch, it looks quite sane, so I guess the failed
compilation for other platforms should be investigated further (I've
put Laurent in the TO: field because of this).

Cheers,
Ivan

[1] https://cgit.kde.org/kcoreaddons.git/log/


On Fri, Apr 20, 2018 at 10:49 AM, Ben Cooksley  wrote:
> Git commit bf353edc42fdecf9fda34a024ba7cabcd0efaf68 by Ben Cooksley.
> Committed on 20/04/2018 at 08:47.
> Pushed by bcooksley into branch 'master'.
>
> Revert "Warning--"
>
> The changes in these commits fail on all platforms aside from Linux, breaking 
> the FreeBSD, Android and Windows builds.
>
> CCMAIL: kde-frameworks-devel@kde.org
>
> This reverts commit 6c27d32fd88387a399955e4497eb976bffc0780e.
>
> M  +1-1src/lib/io/kdirwatch.h
> M  +1-1src/lib/jobs/kjobtrackerinterface.h
> M  +0-1src/lib/kaboutdata.cpp
> M  +2-2src/lib/util/kformatprivate.cpp
>
> https://commits.kde.org/kcoreaddons/bf353edc42fdecf9fda34a024ba7cabcd0efaf68
>
> diff --git a/src/lib/io/kdirwatch.h b/src/lib/io/kdirwatch.h
> index d25f2e9..e0de0b6 100644
> --- a/src/lib/io/kdirwatch.h
> +++ b/src/lib/io/kdirwatch.h
> @@ -86,7 +86,7 @@ public:
>   * is added.
>   * @param parent the parent of the QObject (or 0 for parent-less 
> KDataTools)
>   */
> -explicit KDirWatch(QObject *parent = nullptr);
> +KDirWatch(QObject *parent = nullptr);
>
>  /**
>   * Destructor.
> diff --git a/src/lib/jobs/kjobtrackerinterface.h 
> b/src/lib/jobs/kjobtrackerinterface.h
> index 4c76ca9..f47dfc9 100644
> --- a/src/lib/jobs/kjobtrackerinterface.h
> +++ b/src/lib/jobs/kjobtrackerinterface.h
> @@ -41,7 +41,7 @@ public:
>   *
>   * @param parent the parent object
>   */
> -explicit KJobTrackerInterface(QObject *parent = nullptr);
> +KJobTrackerInterface(QObject *parent = nullptr);
>
>  /**
>   * Destroys a KJobTrackerInterface
> diff --git a/src/lib/kaboutdata.cpp b/src/lib/kaboutdata.cpp
> index bf4e505..194becf 100644
> --- a/src/lib/kaboutdata.cpp
> +++ b/src/lib/kaboutdata.cpp
> @@ -286,7 +286,6 @@ QString KAboutLicense::text() const
>  result = d->_licenseText;
>  break;
>  }
> -Q_FALLTHROUGH()
>  // fall through
>  default:
>  result += QCoreApplication::translate(
> diff --git a/src/lib/util/kformatprivate.cpp b/src/lib/util/kformatprivate.cpp
> index 3e11c84..c64917d 100644
> --- a/src/lib/util/kformatprivate.cpp
> +++ b/src/lib/util/kformatprivate.cpp
> @@ -372,7 +372,7 @@ QString KFormatPrivate::formatRelativeDate(const QDate 
> &date, QLocale::FormatTyp
>  return tr("Invalid date", "used when a relative date string can't be 
> generated because the date is invalid");
>  }
>
> -const qint64 daysTo = QDate::currentDate().daysTo(date);
> +const int daysTo = QDate::currentDate().daysTo(date);
>  if (daysTo > 7 || daysTo < -7) {
>  return m_locale.toString(date, format);
>  }
> @@ -426,7 +426,7 @@ QString KFormatPrivate::formatRelativeDate(const QDate 
> &date, QLocale::FormatTyp
>
>  QString KFormatPrivate::formatRelativeDateTime(const QDateTime &dateTime, 
> QLocale::FormatType format) const
>  {
> -const qint64 daysTo = QDate::currentDate().daysTo(dateTime.date());
> +int daysTo = QDate::currentDate().daysTo(dateTime.date());
>  if (daysTo > 7 || daysTo < -7) {
>  return m_locale.toString(dateTime, format);
>  }



-- 
KDE, ivan.cu...@kde.org, http://cukic.co/
gpg key fingerprint: 292F 9B5C 5A1B 2A2F 9CF3  45DF C9C5 77AF 0A37 240A

Re: Experimental class in baloo

[Ivan Čukić]

p.s. It is common for the pimpl classes in Qt and KF5 to be named
ClassName::Private or ClassNamePrivate and for the pointer to be named
`d` - not ClassNameImpl and m_pimpl.

p.p.s. I've seen that the DatabaseSanitizer is exported in
libKF5BalooEngine.so.5 - if this is a library meant for 3rd party
plugins or applications to be linked against, then David's comment was
right.



On Thu, Mar 22, 2018 at 6:49 PM, Ivan Čukić  wrote:
> Hi,
>
> Is this a class that is supposed to be available in the library (.so
> file) - to be used in applications that use baloo, or is it only a
> class in the backend?
>
> If it is the second case (which I guess it is since I don't see the
> symbols exported in libKF5Baloo.so), then this should not be a problem
> - it is not a part of the API nor the ABI.
>
> Cheers,
> Ivan
>
>
> On Wed, Mar 21, 2018 at 3:03 PM, Michael Heidelbach  wrote:
>> On 21.03.2018 01:16, David Edmundson wrote:
>>
>>
>>
>> On Tue, Mar 20, 2018 at 9:43 AM, Michael Heidelbach 
>> wrote:
>>>
>>> Hi!
>>>
>>> I've recently introduced a new class for baloo. It is mainly for
>>> debugging. As it is accompanied with a command line tool it may be useful
>>> for users too.
>>>
>>> It is still in an experimental state and it's likely I'll wish to change
>>> the public interface.
>>>
>>> 1) Is it possible to mark that class as experimental for some time and
>>> have the allowance to chance the interface?
>>
>>
>> Once you've exported symbols in the same library as baloo...not really.
>>>
>>> 2) If so, what is the best way to communicate that?
>>
>> If it's purely for debugging, stick it behind some optional CMake definition
>> so only users who explicitly enable it have the header installed.
>>
>>
>> David
>>
>> Thank you for your reply, David.
>> In this case I would like to hide it (and the command line tool) at least
>> until 5.46.
>> I'm not very knowledgeable with CMake. I guess sticking it 'behind some
>> optional CMake definition' will also account for it not being part of the
>> library. I've never done this before can you some help or an example please.
>>
>> Michael
>>
>>
>
>
>
> --
> KDE, ivan.cu...@kde.org, http://cukic.co/
> gpg key fingerprint: 292F 9B5C 5A1B 2A2F 9CF3  45DF C9C5 77AF 0A37 240A



-- 
KDE, ivan.cu...@kde.org, http://cukic.co/
gpg key fingerprint: 292F 9B5C 5A1B 2A2F 9CF3  45DF C9C5 77AF 0A37 240A

Re: Experimental class in baloo

[Ivan Čukić]

Hi,

Is this a class that is supposed to be available in the library (.so
file) - to be used in applications that use baloo, or is it only a
class in the backend?

If it is the second case (which I guess it is since I don't see the
symbols exported in libKF5Baloo.so), then this should not be a problem
- it is not a part of the API nor the ABI.

Cheers,
Ivan


On Wed, Mar 21, 2018 at 3:03 PM, Michael Heidelbach  wrote:
> On 21.03.2018 01:16, David Edmundson wrote:
>
>
>
> On Tue, Mar 20, 2018 at 9:43 AM, Michael Heidelbach 
> wrote:
>>
>> Hi!
>>
>> I've recently introduced a new class for baloo. It is mainly for
>> debugging. As it is accompanied with a command line tool it may be useful
>> for users too.
>>
>> It is still in an experimental state and it's likely I'll wish to change
>> the public interface.
>>
>> 1) Is it possible to mark that class as experimental for some time and
>> have the allowance to chance the interface?
>
>
> Once you've exported symbols in the same library as baloo...not really.
>>
>> 2) If so, what is the best way to communicate that?
>
> If it's purely for debugging, stick it behind some optional CMake definition
> so only users who explicitly enable it have the header installed.
>
>
> David
>
> Thank you for your reply, David.
> In this case I would like to hide it (and the command line tool) at least
> until 5.46.
> I'm not very knowledgeable with CMake. I guess sticking it 'behind some
> optional CMake definition' will also account for it not being part of the
> library. I've never done this before can you some help or an example please.
>
> Michael
>
>



-- 
KDE, ivan.cu...@kde.org, http://cukic.co/
gpg key fingerprint: 292F 9B5C 5A1B 2A2F 9CF3  45DF C9C5 77AF 0A37 240A

Re: D11193: Sonnet : use current hunspell API

[Ivan Čukić]

You should probably add the sonnet maintainer (or whoever committed
the most to the repository in recent months) as the reviewer.

Cheers,
Ivan

On Tue, Mar 13, 2018 at 10:26 AM, René J. V.  Bertin
 wrote:
> Silence means acceptance?
>



-- 
KDE, ivan.cu...@kde.org, http://cukic.co/
gpg key fingerprint: 292F 9B5C 5A1B 2A2F 9CF3  45DF C9C5 77AF 0A37 240A

D10691: [kactivities-stats] Fix plasmashell crash when database is broken

[Ivan Čukić]

ivan added a comment.


  The fix needs to be a bit more complex. We can not really rely on the user to 
fix the database.
  
  The current plan is to have kamd backup the important parts of the database 
from time to time, and to recreate the database when needed.
  
  I'll see whether I can roll out a crash=>fail-with-a-message patch for the 
LTS.
  
  The current implementation relied on my (wrong) assumption that sqlite is 
very stable and that it can not go berserk and corrupt the database. :)

REPOSITORY
  R159 KActivities Statistics

REVISION DETAIL
  https://phabricator.kde.org/D10691

To: kpiwowarski, #plasma, #frameworks, hein, ivan
Cc: ivan, plasma-devel, #frameworks, michaelh, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D10691: [kactivities-stats] Fix plasmashell crash when database is broken

[Ivan Čukić]

ivan requested changes to this revision.
ivan added a comment.
This revision now requires changes to proceed.


  Well, in a library that 99% relies on a database, having no database is not 
something that can be gracefully handled (the returned nullptr is asserted on 
as well in the function that calls ::instance in ResultSet :) - because of 
this, I'm changing to 'Request Changes').
  
  Graceful handling instead of qFatal/Q_ASSERT could be implemented so that all 
models and queries return empty sets. Which would be useless, but as I said, I 
don't have anything against that.
  
  When we get the bug reports about empty favourites, there will be an 
additional test step "Hi, do you have this message in the plasmashell output 
'Database can not be opened...'" :)

REPOSITORY
  R159 KActivities Statistics

REVISION DETAIL
  https://phabricator.kde.org/D10691

To: kpiwowarski, #plasma, #frameworks, hein, ivan
Cc: ivan, plasma-devel, #frameworks, michaelh, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D10691: [kactivities-stats] Fix plasmashell crash when database is broken

[Ivan Čukić]

ivan added a comment.


  @hein
  
  This is not really as simple as 'accept without a comment'.
  
  I don't mind this patch to be merged, but keep in mind that it just turns 
users for which plasma crashes (for which we get an explicit message as to why) 
into users who do not have certain parts of Plasma working where the error 
message will get lost in the Plasma's log. Now, as a maintainer of said parts 
of Plasma, that is your call.

REPOSITORY
  R159 KActivities Statistics

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D10691

To: kpiwowarski, #plasma, #frameworks, hein
Cc: ivan, plasma-devel, #frameworks, michaelh, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D4906: Fix kactivities-stats into tier3

[Ivan Čukić]

ivan accepted this revision.
ivan added a comment.
This revision is now accepted and ready to land.


  This is a good point. *
  
  I'll make this lower again when I remove KConfig from KActivities/imports.
  
  *Though, KActivities imports are tier 2 (the KIO dep was removed some time 
ago - forgot to remove it from CMakeLists.txt).

REPOSITORY
  R159 KActivities Statistics

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D4906

To: apol, #frameworks, ivan
Cc: adridg

D4847: KAuth integration in document saving

[Ivan Čukić]

ivan added subscribers: lbeltrame, ivan.
ivan added a comment.


  @lbeltrame
  
  I don't really see 'reasoning' in that report more than something that sounds 
like 'polkit is a hack to replace suid for setting the system time and time 
zone'.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D4847

To: martinkostolny, dhaumann, #ktexteditor, dfaure
Cc: ivan, lbeltrame, fvogt, apol, dfaure, anthonyfieroni, cullmann, ltoscano, 
dhaumann, graesslin, davidedmundson, palant, kwrite-devel, #frameworks, head7, 
kfunk, sars

D4633: WIP: Updated folder decrypted and encrypted icons

[Ivan Čukić]

ivan added a comment.


  I didn't want to change the current icon that much.

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D4633

To: ivan, andreaska, alex-l
Cc: #frameworks

D4906: Fix kactivities-stats into tier3

[Ivan Čukić]

ivan added a comment.
View RevisionKActivities is tier 1 even if the metainfo.yaml says otherwise. It only requires boost.

So, this should be changed in KActivities, not in KActivitiesStatsREPOSITORYR159 KActivities StatisticsREVISION DETAILhttps://phabricator.kde.org/D4906EMAIL PREFERENCEShttps://phabricator.kde.org/settings/panel/emailpreferences/To: apol, Frameworks, ivan

Re: Differential e-mail subject re-arrangement

[Ivan Čukić]

>   [Differential] D4508: Plasma controls based on QtQuickControls2 [Commented 
> On]
>
> I personally find that having a "vertical" line in which all the subjects of 
> the differential emails start makes it much easier to read.

+1 I'd even shorten The 'Differential' part of it to just 'Diff'.

Cheers,
Ivan


--

KDE, ivan.cu...@kde.org, http://cukic.co/
gpg key fingerprint: 8FE4 D32F 7061 EA9C 8232  07AE 01C6 CE2B FF04 1C12

[Differential] [Commented On] D4633: WIP: Updated folder decrypted and encrypted icons

[Ivan Čukić]

ivan added a comment.


  Or a bit different:
  
  F2501511: icons-again.png 

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D4633

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: ivan, alex-l, andreaska
Cc: #frameworks

[Differential] [Updated] D4633: WIP: Updated folder decrypted and encrypted icons

[Ivan Čukić]

ivan retitled this revision from "Updated folder decrypted and encrypted icons" 
to "WIP: Updated folder decrypted and encrypted icons".
ivan updated the summary for this revision.

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D4633

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: ivan, alex-l, andreaska
Cc: #frameworks

[Differential] [Updated, 152 lines] D4633: Updated folder decrypted and encrypted icons

[Ivan Čukić]

ivan updated this revision to Diff 11410.
ivan added a comment.


  - Small issue with line borders

REPOSITORY
  R266 Breeze Icons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D4633?vs=11409&id=11410

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D4633

AFFECTED FILES
  icons/places/64/folder-decrypted.svg
  icons/places/64/folder-encrypted.svg

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: ivan, alex-l, andreaska
Cc: #frameworks

[Differential] [Updated] D4633: Updated folder decrypted and encrypted icons

[Ivan Čukić]

ivan updated the summary for this revision.

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D4633

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: ivan, alex-l, andreaska
Cc: #frameworks

[Differential] [Request, 159 lines] D4633: Updated folder decrypted and encrypted icons

[Ivan Čukić]

ivan created this revision.
ivan added reviewers: alex-l, andreaska.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.

REVISION SUMMARY
  The current icons have a nice idea, but the horizontally-cut
  
checkers look a bit like a rendering glitch

REPOSITORY
  R266 Breeze Icons

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D4633

AFFECTED FILES
  icons/places/64/folder-decrypted.svg
  icons/places/64/folder-encrypted.svg

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: ivan, alex-l, andreaska
Cc: #frameworks

[Differential] [Commented On] D4626: Renamed icons for encrypted and decripted folders

[Ivan Čukić]

ivan added a comment.


  I will use them, but don't do that just yet - I want to propose some changes 
to them.

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D4626

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: ivan, alex-l, andreaska
Cc: #frameworks

[Differential] [Closed] D4626: Renamed icons for encrypted and decripted folders

[Ivan Čukić]

This revision was automatically updated to reflect the committed changes.
Closed by commit R266:b79085c027c0: Renamed icons for encrypted and decripted 
folders (authored by ivan).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D4626?vs=11385&id=11386#toc

REPOSITORY
  R266 Breeze Icons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D4626?vs=11385&id=11386

REVISION DETAIL
  https://phabricator.kde.org/D4626

AFFECTED FILES
  icons/places/64/folder-decrypt.svg
  icons/places/64/folder-decrypted.svg
  icons/places/64/folder-encrypt.svg
  icons/places/64/folder-encrypted.svg

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: ivan, alex-l, andreaska
Cc: #frameworks

[Differential] [Request, 958 lines] D4626: Renamed icons for encrypted and decripted folders

[Ivan Čukić]

ivan created this revision.
ivan added reviewers: alex-l, andreaska.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.

REVISION SUMMARY
  Since the icons are in places, not in actions, changed the names
  to imply state, not action.

REPOSITORY
  R266 Breeze Icons

BRANCH
  ivan/rename-icons

REVISION DETAIL
  https://phabricator.kde.org/D4626

AFFECTED FILES
  icons/places/64/folder-decrypt.svg
  icons/places/64/folder-decrypted.svg
  icons/places/64/folder-encrypt.svg
  icons/places/64/folder-encrypted.svg

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: ivan, alex-l, andreaska
Cc: #frameworks

[Differential] [Request, 799 lines] D4625: Icons for Plasma Vault

[Ivan Čukić]

ivan created this revision.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.

REVISION SUMMARY
  Merge branch 'master' into ivan/plasmavault-icons

REPOSITORY
  R266 Breeze Icons

BRANCH
  ivan/plasmavault-icons

REVISION DETAIL
  https://phabricator.kde.org/D4625

AFFECTED FILES
  icons/apps/64/plasmavault-error.svg
  icons/apps/64/plasmavault.svg

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: ivan
Cc: #frameworks

[Differential] [Closed] D4624: Added arc config file

[Ivan Čukić]

This revision was automatically updated to reflect the committed changes.
Closed by commit R266:bc60e3c2f85f: Added arc config file (authored by ivan).

REPOSITORY
  R266 Breeze Icons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D4624?vs=11382&id=11383

REVISION DETAIL
  https://phabricator.kde.org/D4624

AFFECTED FILES
  .arcconfig

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: ivan, #frameworks

[Differential] [Request, 3 lines] D4624: Added arc config file

[Ivan Čukić]

ivan created this revision.
ivan added a reviewer: Frameworks.
Restricted Application added a project: Frameworks.

REVISION SUMMARY
  Added arc configuration file

REPOSITORY
  R266 Breeze Icons

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D4624

AFFECTED FILES
  .arcconfig

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: ivan, #frameworks

[Differential] [Accepted] D3850: Pass -fno-operator-names when supported

[Ivan Čukić]

ivan accepted this revision.
ivan added a comment.
This revision is now accepted and ready to land.


  Fine by me.

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D3850

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: kfunk, #frameworks, #buildsystem, ivan
Cc: rakuco, elvisangelaccio

Re: To C++11 or not?

[Ivan Čukić]

Hi all,

I find it strange (not to say disappointing) that we are even talking
about C++11 in 2016.

+1 for the proposal.

I wanted to start discussions about C++14 (the "bugfix" release of
C++), but I'll wait until this discussion is finished. :)

Cheers,
Ivan

Re: KDE Frameworks & Plasma 5.8 LTS

[Ivan Čukić]

> 2. We provide KF 5.26.1

This would mean that we should also provide 5.27.1 (and .2, .3) for the
distros that use KF 5.27 with the LTS release.

I'd love to have distros to **properly update KF** during the 5.8 LTS life,
but if we can not get them to do so, I agree that we need to find an
alternative.

Keeping a list of bug-fix patches is one option, but we would need to keep
the list for each future release of KF which might be problematic. But,
if we had a common place to put them, it might be an OK option.

The same would go for the point releases. It would require some additional
work to tag the point releases in Git.

I think the list of patches might be easier to pull off -- we would not
need to provide releases for all frameworks -- just for those that need
to be updated. This way, the distros would know which ones need updating
at all.

Cheers,
Ivan

--

KDE, ivan.cu...@kde.org, http://cukic.co/
gpg key fingerprint: 8FE4 D32F 7061 EA9C 8232  07AE 01C6 CE2B FF04 1C12

Re: Review Request 128567: Add a convenient API to query the windowing system/platform used by Qt

[Ivan Čukić]

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


Ship it!




Ship It!

- Ivan Čukić


On Aug. 2, 2016, 5:47 a.m., Martin Gräßlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128567/
> ---
> 
> (Updated Aug. 2, 2016, 5:47 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kwindowsystem
> 
> 
> Description
> ---
> 
> A common pattern for our applications which have to do platform specific
> tasks is to query the used platform with code like:
> if (QX11Info::isPlatformX11()) {
> // do X11 specific stuff
> } else if 
> (QGuiApplication::platformName().startsWith(QLatin1String("wayland"))) {
> // do Wayland specific stuff
> }
> 
> The problem with that is that it involves string comparisons and is easy
> to get wrong. E.g. for X11 one should use the QX11Info helper, for
> Wayland one has to compare with startsWith. There is lot of domain
> specific knowledge going into it to make it work correctly and in an
> efficient way.
> 
> This change introduces an enum based API which encapsulates the
> knowledge: it does the proper comparisons and caches the result, so
> that one does not need to do string comparisons again and again.
> 
> So far the implementation supports the platforms X11/XCB and Wayland.
> In addition there are convenient methods to check for a specific
> platform modelled after QX11Info::isPlatformX11.
> 
> Further platforms can be added by the maintainers of the respective
> platforms.
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt 65ed8d4da50fd41dd3060576284456c83c3629ab 
>   autotests/helper/CMakeLists.txt PRE-CREATION 
>   autotests/helper/wayland_platform.cpp PRE-CREATION 
>   autotests/kwindowsystem_platform_wayland_test.cpp PRE-CREATION 
>   autotests/kwindowsystemx11test.cpp cdf94e3f6573faea6dfd6038dded3ee1269e2552 
>   src/kwindowsystem.h 551b01f5831e2fbdf0d0ad30f3debf0955da189e 
>   src/kwindowsystem.cpp 4dfcce8bbfeed09f3f6ddec10a1eb5fd2d418313 
> 
> Diff: https://git.reviewboard.kde.org/r/128567/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Martin Gräßlin
> 
>

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

Re: Review Request 128567: Add a convenient API to query the windowing system/platform used by Qt

[Ivan Čukić]

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




src/kwindowsystem.h (line 625)
<https://git.reviewboard.kde.org/r/128567/#comment66004>

why not `static inline`?



src/kwindowsystem.cpp (line 731)
<https://git.reviewboard.kde.org/r/128567/#comment66003>

Why not just:

```
static Platform s_platform = initPlatform()
```

It would be thread-safe and shorter. Or am I missing something?


- Ivan Čukić


On Aug. 1, 2016, 3:27 p.m., Martin Gräßlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128567/
> ---
> 
> (Updated Aug. 1, 2016, 3:27 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kwindowsystem
> 
> 
> Description
> ---
> 
> A common pattern for our applications which have to do platform specific
> tasks is to query the used platform with code like:
> if (QX11Info::isPlatformX11()) {
> // do X11 specific stuff
> } else if 
> (QGuiApplication::platformName().startsWith(QLatin1String("wayland"))) {
> // do Wayland specific stuff
> }
> 
> The problem with that is that it involves string comparisons and is easy
> to get wrong. E.g. for X11 one should use the QX11Info helper, for
> Wayland one has to compare with startsWith. There is lot of domain
> specific knowledge going into it to make it work correctly and in an
> efficient way.
> 
> This change introduces an enum based API which encapsulates the
> knowledge: it does the proper comparisons and caches the result, so
> that one does not need to do string comparisons again and again.
> 
> So far the implementation supports the platforms X11/XCB and Wayland.
> In addition there are convenient methods to check for a specific
> platform modelled after QX11Info::isPlatformX11.
> 
> Further platforms can be added by the maintainers of the respective
> platforms.
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt 65ed8d4da50fd41dd3060576284456c83c3629ab 
>   autotests/helper/CMakeLists.txt PRE-CREATION 
>   autotests/helper/wayland_platform.cpp PRE-CREATION 
>   autotests/kwindowsystem_platform_wayland_test.cpp PRE-CREATION 
>   autotests/kwindowsystemx11test.cpp cdf94e3f6573faea6dfd6038dded3ee1269e2552 
>   src/kwindowsystem.h 551b01f5831e2fbdf0d0ad30f3debf0955da189e 
>   src/kwindowsystem.cpp 4dfcce8bbfeed09f3f6ddec10a1eb5fd2d418313 
> 
> Diff: https://git.reviewboard.kde.org/r/128567/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Martin Gräßlin
> 
>

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

Re: Review Request 128466: Rename Checksums tab to Integrity

[Ivan Čukić]

> On July 18, 2016, 12:05 p.m., Sebastian Kügler wrote:
> > Please don't ship it, yet.
> > 
> > 
> > I find the UI illogical. There's a groupbox grouping the checksum buttons, 
> > but then you can input the checksum above, so essentially, the groupbox is 
> > unnecessary and confusing.
> > 
> > Perhaps the whole thing could be simplified by naming the tab "Checksums" 
> > and removing the groupbox altogether, as it's not providing any semantic 
> > value.
> > 
> > A usability reviewer should have a look.
> 
> Kai Uwe Broulik wrote:
> This dialog has been created in Review 128283 and Usability has been 
> involved from the beginning...
> 
> Sebastian Kügler wrote:
> It has changed in a significant way, though, making it illogical.
> 
> (Not that I understand the "Share" title in the original review, but 
> that's another matter.)
> 
> This needs more work.
> 
> Elvis Angelaccio wrote:
> > Perhaps the whole thing could be simplified by naming the tab 
> "Checksums" and removing the groupbox altogether, as it's not providing any 
> semantic value.
> 
> Preview here: https://share.kde.org/index.php/s/RUs9gAhIQqpFIqF
> 
> Sebastian Kügler wrote:
> This looks logical to me, and it's simpler: Very good!
> 
> (Take that as "sebas withdraws his objection" :))
> 
> Thomas Pfeiffer wrote:
> Clear -1 to removing the group box.
> 
> Here is tha rationale:
> For most "regular" users, only the lineedit at the top is relevant. The 
> calculate stuff is just distraction and - worse - potential confusion. If we 
> remove any visual distinction between the two, we just make the situation 
> worse for them.
> 
> I agree that calling the tab "Checksums" is still better, though, because 
> "Integrity" is too vague.
> 
> For the "average user" I just re-tested this with, what would actually 
> help her is creating a second box around the verification part, calling the 
> top one "Verify checksum" and the bottom "Calculate checksums".
> That way if she was told e.g. by a website to verify a checksum, she'd 
> knew she could simply ignore the whole calculation part.
> 
> Overall simplicity should not be the top priority here: The priority 
> should be to make it clear to users who just want to check whether a download 
> went okay which part they should care about and which they can ignore, while 
> still providing the calculation part for advanced users who want to do that.
> 
> Of course another option would be to split it in two tabs, but that might 
> make the tab bar quite long especially in languages like German.
> 
> Sebastian Kügler wrote:
> The latter part seems redundant then. How can you verify a checksum 
> without calculating it?
> 
> Thomas Pfeiffer wrote:
> See _that_ is why the bottom box was originally called "Share". Of course 
> the verification part also does calculation, but it does so without you 
> telling it to. You paste in the checksum, and it tells you whether it matches 
> or not.
> The manual calculation at the bottom is for people who share a file with 
> others and want to accompany it with a checksum for _them_ to verify it.
> 
> I think we might get away with "Calculate" anyway because those who don#t 
> know much about checksums don't need to know that the verify part calculates 
> as well, and those who know it should still be able to use the thing 
> correctly.
> 
> Sebastian Kügler wrote:
> That 'Share' title completely puzzled me, and I think I'm the kind of 
> user this dialog should work for very well. (I need to verify checksums all 
> the time.)
> 
> To be quite honest, from getting it explained, I get the strong 
> impression that you're overthinking it.
> 
> To me, the most logical would be:
> 
> * Calculate checksums at the top
> * Under that, the input field so I can c/p or type my checksum in there 
> to have it compared automatically. 
> 
> That's both, the order of the workflow as well as the logical order of 
> operation. 'Calculate' underneath would raise exactly the same question as I 
> put above: "...but but but ... it could not verify it without calculating it, 
> yet I have to hit a button to calculate ... maybe I should try again and 
> maybe I should just use the commandline to be sure". 
> 
> Point in case: for this kind of stuff, simplicity trumps since it makes

Re: Review Request 128466: Rename Checksums tab to Integrity

[Ivan Čukić]

> On July 18, 2016, 12:05 p.m., Sebastian Kügler wrote:
> > Please don't ship it, yet.
> > 
> > 
> > I find the UI illogical. There's a groupbox grouping the checksum buttons, 
> > but then you can input the checksum above, so essentially, the groupbox is 
> > unnecessary and confusing.
> > 
> > Perhaps the whole thing could be simplified by naming the tab "Checksums" 
> > and removing the groupbox altogether, as it's not providing any semantic 
> > value.
> > 
> > A usability reviewer should have a look.
> 
> Kai Uwe Broulik wrote:
> This dialog has been created in Review 128283 and Usability has been 
> involved from the beginning...
> 
> Sebastian Kügler wrote:
> It has changed in a significant way, though, making it illogical.
> 
> (Not that I understand the "Share" title in the original review, but 
> that's another matter.)
> 
> This needs more work.
> 
> Elvis Angelaccio wrote:
> > Perhaps the whole thing could be simplified by naming the tab 
> "Checksums" and removing the groupbox altogether, as it's not providing any 
> semantic value.
> 
> Preview here: https://share.kde.org/index.php/s/RUs9gAhIQqpFIqF
> 
> Sebastian Kügler wrote:
> This looks logical to me, and it's simpler: Very good!
> 
> (Take that as "sebas withdraws his objection" :))
> 
> Thomas Pfeiffer wrote:
> Clear -1 to removing the group box.
> 
> Here is tha rationale:
> For most "regular" users, only the lineedit at the top is relevant. The 
> calculate stuff is just distraction and - worse - potential confusion. If we 
> remove any visual distinction between the two, we just make the situation 
> worse for them.
> 
> I agree that calling the tab "Checksums" is still better, though, because 
> "Integrity" is too vague.
> 
> For the "average user" I just re-tested this with, what would actually 
> help her is creating a second box around the verification part, calling the 
> top one "Verify checksum" and the bottom "Calculate checksums".
> That way if she was told e.g. by a website to verify a checksum, she'd 
> knew she could simply ignore the whole calculation part.
> 
> Overall simplicity should not be the top priority here: The priority 
> should be to make it clear to users who just want to check whether a download 
> went okay which part they should care about and which they can ignore, while 
> still providing the calculation part for advanced users who want to do that.
> 
> Of course another option would be to split it in two tabs, but that might 
> make the tab bar quite long especially in languages like German.
> 
> Sebastian Kügler wrote:
> The latter part seems redundant then. How can you verify a checksum 
> without calculating it?
> 
> Thomas Pfeiffer wrote:
> See _that_ is why the bottom box was originally called "Share". Of course 
> the verification part also does calculation, but it does so without you 
> telling it to. You paste in the checksum, and it tells you whether it matches 
> or not.
> The manual calculation at the bottom is for people who share a file with 
> others and want to accompany it with a checksum for _them_ to verify it.
> 
> I think we might get away with "Calculate" anyway because those who don#t 
> know much about checksums don't need to know that the verify part calculates 
> as well, and those who know it should still be able to use the thing 
> correctly.
> 
> Sebastian Kügler wrote:
> That 'Share' title completely puzzled me, and I think I'm the kind of 
> user this dialog should work for very well. (I need to verify checksums all 
> the time.)
> 
> To be quite honest, from getting it explained, I get the strong 
> impression that you're overthinking it.
> 
> To me, the most logical would be:
> 
> * Calculate checksums at the top
> * Under that, the input field so I can c/p or type my checksum in there 
> to have it compared automatically. 
> 
> That's both, the order of the workflow as well as the logical order of 
> operation. 'Calculate' underneath would raise exactly the same question as I 
> put above: "...but but but ... it could not verify it without calculating it, 
> yet I have to hit a button to calculate ... maybe I should try again and 
> maybe I should just use the commandline to be sure". 
> 
> Point in case: for this kind of stuff, simplicity trumps since it makes

Re: Review Request 128466: Rename Checksums tab to Integrity

[Ivan Čukić]

> On July 18, 2016, 12:05 p.m., Sebastian Kügler wrote:
> > Please don't ship it, yet.
> > 
> > 
> > I find the UI illogical. There's a groupbox grouping the checksum buttons, 
> > but then you can input the checksum above, so essentially, the groupbox is 
> > unnecessary and confusing.
> > 
> > Perhaps the whole thing could be simplified by naming the tab "Checksums" 
> > and removing the groupbox altogether, as it's not providing any semantic 
> > value.
> > 
> > A usability reviewer should have a look.
> 
> Kai Uwe Broulik wrote:
> This dialog has been created in Review 128283 and Usability has been 
> involved from the beginning...
> 
> Sebastian Kügler wrote:
> It has changed in a significant way, though, making it illogical.
> 
> (Not that I understand the "Share" title in the original review, but 
> that's another matter.)
> 
> This needs more work.
> 
> Elvis Angelaccio wrote:
> > Perhaps the whole thing could be simplified by naming the tab 
> "Checksums" and removing the groupbox altogether, as it's not providing any 
> semantic value.
> 
> Preview here: https://share.kde.org/index.php/s/RUs9gAhIQqpFIqF
> 
> Sebastian Kügler wrote:
> This looks logical to me, and it's simpler: Very good!
> 
> (Take that as "sebas withdraws his objection" :))
> 
> Thomas Pfeiffer wrote:
> Clear -1 to removing the group box.
> 
> Here is tha rationale:
> For most "regular" users, only the lineedit at the top is relevant. The 
> calculate stuff is just distraction and - worse - potential confusion. If we 
> remove any visual distinction between the two, we just make the situation 
> worse for them.
> 
> I agree that calling the tab "Checksums" is still better, though, because 
> "Integrity" is too vague.
> 
> For the "average user" I just re-tested this with, what would actually 
> help her is creating a second box around the verification part, calling the 
> top one "Verify checksum" and the bottom "Calculate checksums".
> That way if she was told e.g. by a website to verify a checksum, she'd 
> knew she could simply ignore the whole calculation part.
> 
> Overall simplicity should not be the top priority here: The priority 
> should be to make it clear to users who just want to check whether a download 
> went okay which part they should care about and which they can ignore, while 
> still providing the calculation part for advanced users who want to do that.
> 
> Of course another option would be to split it in two tabs, but that might 
> make the tab bar quite long especially in languages like German.
> 
> Sebastian Kügler wrote:
> The latter part seems redundant then. How can you verify a checksum 
> without calculating it?
> 
> Thomas Pfeiffer wrote:
> See _that_ is why the bottom box was originally called "Share". Of course 
> the verification part also does calculation, but it does so without you 
> telling it to. You paste in the checksum, and it tells you whether it matches 
> or not.
> The manual calculation at the bottom is for people who share a file with 
> others and want to accompany it with a checksum for _them_ to verify it.
> 
> I think we might get away with "Calculate" anyway because those who don#t 
> know much about checksums don't need to know that the verify part calculates 
> as well, and those who know it should still be able to use the thing 
> correctly.
> 
> Sebastian Kügler wrote:
> That 'Share' title completely puzzled me, and I think I'm the kind of 
> user this dialog should work for very well. (I need to verify checksums all 
> the time.)
> 
> To be quite honest, from getting it explained, I get the strong 
> impression that you're overthinking it.
> 
> To me, the most logical would be:
> 
> * Calculate checksums at the top
> * Under that, the input field so I can c/p or type my checksum in there 
> to have it compared automatically. 
> 
> That's both, the order of the workflow as well as the logical order of 
> operation. 'Calculate' underneath would raise exactly the same question as I 
> put above: "...but but but ... it could not verify it without calculating it, 
> yet I have to hit a button to calculate ... maybe I should try again and 
> maybe I should just use the commandline to be sure". 
> 
> Point in case: for this kind of stuff, simplicity trumps since it makes

Re: Review Request 128466: Rename Checksums tab to Integrity

[Ivan Čukić]

> On July 18, 2016, 12:05 p.m., Sebastian Kügler wrote:
> > Please don't ship it, yet.
> > 
> > 
> > I find the UI illogical. There's a groupbox grouping the checksum buttons, 
> > but then you can input the checksum above, so essentially, the groupbox is 
> > unnecessary and confusing.
> > 
> > Perhaps the whole thing could be simplified by naming the tab "Checksums" 
> > and removing the groupbox altogether, as it's not providing any semantic 
> > value.
> > 
> > A usability reviewer should have a look.
> 
> Kai Uwe Broulik wrote:
> This dialog has been created in Review 128283 and Usability has been 
> involved from the beginning...
> 
> Sebastian Kügler wrote:
> It has changed in a significant way, though, making it illogical.
> 
> (Not that I understand the "Share" title in the original review, but 
> that's another matter.)
> 
> This needs more work.
> 
> Elvis Angelaccio wrote:
> > Perhaps the whole thing could be simplified by naming the tab 
> "Checksums" and removing the groupbox altogether, as it's not providing any 
> semantic value.
> 
> Preview here: https://share.kde.org/index.php/s/RUs9gAhIQqpFIqF
> 
> Sebastian Kügler wrote:
> This looks logical to me, and it's simpler: Very good!
> 
> (Take that as "sebas withdraws his objection" :))
> 
> Thomas Pfeiffer wrote:
> Clear -1 to removing the group box.
> 
> Here is tha rationale:
> For most "regular" users, only the lineedit at the top is relevant. The 
> calculate stuff is just distraction and - worse - potential confusion. If we 
> remove any visual distinction between the two, we just make the situation 
> worse for them.
> 
> I agree that calling the tab "Checksums" is still better, though, because 
> "Integrity" is too vague.
> 
> For the "average user" I just re-tested this with, what would actually 
> help her is creating a second box around the verification part, calling the 
> top one "Verify checksum" and the bottom "Calculate checksums".
> That way if she was told e.g. by a website to verify a checksum, she'd 
> knew she could simply ignore the whole calculation part.
> 
> Overall simplicity should not be the top priority here: The priority 
> should be to make it clear to users who just want to check whether a download 
> went okay which part they should care about and which they can ignore, while 
> still providing the calculation part for advanced users who want to do that.
> 
> Of course another option would be to split it in two tabs, but that might 
> make the tab bar quite long especially in languages like German.
> 
> Sebastian Kügler wrote:
> The latter part seems redundant then. How can you verify a checksum 
> without calculating it?
> 
> Thomas Pfeiffer wrote:
> See _that_ is why the bottom box was originally called "Share". Of course 
> the verification part also does calculation, but it does so without you 
> telling it to. You paste in the checksum, and it tells you whether it matches 
> or not.
> The manual calculation at the bottom is for people who share a file with 
> others and want to accompany it with a checksum for _them_ to verify it.
> 
> I think we might get away with "Calculate" anyway because those who don#t 
> know much about checksums don't need to know that the verify part calculates 
> as well, and those who know it should still be able to use the thing 
> correctly.
> 
> Sebastian Kügler wrote:
> That 'Share' title completely puzzled me, and I think I'm the kind of 
> user this dialog should work for very well. (I need to verify checksums all 
> the time.)
> 
> To be quite honest, from getting it explained, I get the strong 
> impression that you're overthinking it.
> 
> To me, the most logical would be:
> 
> * Calculate checksums at the top
> * Under that, the input field so I can c/p or type my checksum in there 
> to have it compared automatically. 
> 
> That's both, the order of the workflow as well as the logical order of 
> operation. 'Calculate' underneath would raise exactly the same question as I 
> put above: "...but but but ... it could not verify it without calculating it, 
> yet I have to hit a button to calculate ... maybe I should try again and 
> maybe I should just use the commandline to be sure". 
> 
> Point in case: for this kind of stuff, simplicity trumps since it makes

Re: Review Request 128466: Rename Checksums tab to Integrity

[Ivan Čukić]

> On July 18, 2016, 12:05 p.m., Sebastian Kügler wrote:
> > Please don't ship it, yet.
> > 
> > 
> > I find the UI illogical. There's a groupbox grouping the checksum buttons, 
> > but then you can input the checksum above, so essentially, the groupbox is 
> > unnecessary and confusing.
> > 
> > Perhaps the whole thing could be simplified by naming the tab "Checksums" 
> > and removing the groupbox altogether, as it's not providing any semantic 
> > value.
> > 
> > A usability reviewer should have a look.
> 
> Kai Uwe Broulik wrote:
> This dialog has been created in Review 128283 and Usability has been 
> involved from the beginning...
> 
> Sebastian Kügler wrote:
> It has changed in a significant way, though, making it illogical.
> 
> (Not that I understand the "Share" title in the original review, but 
> that's another matter.)
> 
> This needs more work.
> 
> Elvis Angelaccio wrote:
> > Perhaps the whole thing could be simplified by naming the tab 
> "Checksums" and removing the groupbox altogether, as it's not providing any 
> semantic value.
> 
> Preview here: https://share.kde.org/index.php/s/RUs9gAhIQqpFIqF
> 
> Sebastian Kügler wrote:
> This looks logical to me, and it's simpler: Very good!
> 
> (Take that as "sebas withdraws his objection" :))
> 
> Thomas Pfeiffer wrote:
> Clear -1 to removing the group box.
> 
> Here is tha rationale:
> For most "regular" users, only the lineedit at the top is relevant. The 
> calculate stuff is just distraction and - worse - potential confusion. If we 
> remove any visual distinction between the two, we just make the situation 
> worse for them.
> 
> I agree that calling the tab "Checksums" is still better, though, because 
> "Integrity" is too vague.
> 
> For the "average user" I just re-tested this with, what would actually 
> help her is creating a second box around the verification part, calling the 
> top one "Verify checksum" and the bottom "Calculate checksums".
> That way if she was told e.g. by a website to verify a checksum, she'd 
> knew she could simply ignore the whole calculation part.
> 
> Overall simplicity should not be the top priority here: The priority 
> should be to make it clear to users who just want to check whether a download 
> went okay which part they should care about and which they can ignore, while 
> still providing the calculation part for advanced users who want to do that.
> 
> Of course another option would be to split it in two tabs, but that might 
> make the tab bar quite long especially in languages like German.
> 
> Sebastian Kügler wrote:
> The latter part seems redundant then. How can you verify a checksum 
> without calculating it?
> 
> Thomas Pfeiffer wrote:
> See _that_ is why the bottom box was originally called "Share". Of course 
> the verification part also does calculation, but it does so without you 
> telling it to. You paste in the checksum, and it tells you whether it matches 
> or not.
> The manual calculation at the bottom is for people who share a file with 
> others and want to accompany it with a checksum for _them_ to verify it.
> 
> I think we might get away with "Calculate" anyway because those who don#t 
> know much about checksums don't need to know that the verify part calculates 
> as well, and those who know it should still be able to use the thing 
> correctly.
> 
> Sebastian Kügler wrote:
> That 'Share' title completely puzzled me, and I think I'm the kind of 
> user this dialog should work for very well. (I need to verify checksums all 
> the time.)
> 
> To be quite honest, from getting it explained, I get the strong 
> impression that you're overthinking it.
> 
> To me, the most logical would be:
> 
> * Calculate checksums at the top
> * Under that, the input field so I can c/p or type my checksum in there 
> to have it compared automatically. 
> 
> That's both, the order of the workflow as well as the logical order of 
> operation. 'Calculate' underneath would raise exactly the same question as I 
> put above: "...but but but ... it could not verify it without calculating it, 
> yet I have to hit a button to calculate ... maybe I should try again and 
> maybe I should just use the commandline to be sure". 
> 
> Point in case: for this kind of stuff, simplicity trumps since it makes