D23112: Add a event Spy for GtkFileChooser recent files

[Ivan Čukić]

ivan added inline comments.

INLINE COMMENTS

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

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

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

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<Bookmark>();
> +    }

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20190815/ce01826d/attachment.html>