Issue 1107 - Support notifications

include/AdblockPlus/Notification.h

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2015 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 #ifndef ADBLOCK_PLUS_NOTIFICATION_H
 #define ADBLOCK_PLUS_NOTIFICATION_H
 
 #include <string>
 #include <vector>
-#include <memory>
+#include "tr1_memory.h"
 
 namespace AdblockPlus
 {
   class FilterEngine;
+  /**
+   * Possible notification types.
+   */
   enum NotificationType
   {
-    NOTIFICATION_TYPE_INFORMATION = 0,

[Felix Dahlke, 2015/01/22 10:25:25]

Why explicitly assign the values here? We typically don't do that with enums.

[sergei, 2015/01/22 14:08:15]

fixed

-    NOTIFICATION_TYPE_QUESTION    = 1,

[Felix Dahlke, 2015/01/22 10:25:25]

No column aligning please :D

-    NOTIFICATION_TYPE_CRITICAL    = 2 
+    NOTIFICATION_TYPE_INFORMATION,
+    NOTIFICATION_TYPE_QUESTION,
+    NOTIFICATION_TYPE_CRITICAL
   };
 
-  struct NotificationTexts

[Felix Dahlke, 2015/01/22 10:25:25]

It's a single one, so "NotificationText"?

[sergei, 2015/01/22 14:08:15]

Actually there were two fields and the plural form is used in javascript
(getLocalizedTexts), anyway it's removed now from here.

[Felix Dahlke, 2015/01/22 14:36:31]

Argh you're right, of course it's two. Should really be NotificationTexts then,
sorry :P

-  {
-    std::string title;
-    std::string message;
-  };
-
-  class Notification : public JsValue, public std::tr1::enable_shared_from_this<Notification>
+  /**
+   * Wrapper for an Adblock Plus notification object.
+   */
+  class Notification: public JsValue
   {
     friend class FilterEngine;
   protected:
-    struct PrivateCtrArg{};

[Felix Dahlke, 2015/01/22 10:25:25]

I still find this pattern highly obscure. We've got a discussion on whether to
use it in libadblockplus elsewhere, let's please keep that there. If we are to
use it, we should use it consistently.

[sergei, 2015/01/22 14:08:15]

JIC, if the constructor is not public then std::make_shared cannot get access to
it.

     static std::tr1::shared_ptr<Notification> JsValueToNotification(const JsValuePtr& jsValue);
-    /// @param jsText is javascript object returned by getLocalizedTexts.
-    static NotificationTexts JsTextsToNotificationTexts(const JsValue& jsText);
+    /**
+     * Constructor.
+     * @param jsValue `JsValuePtr` notification JavaScript object.
+     */
+    explicit Notification(const JsValuePtr& jsValue);
   public:
-    Notification(NotificationType type, const std::string& id, const JsValuePtr& jsValue, PrivateCtrArg);
-    Notification(const JsValuePtr& value, PrivateCtrArg);
-    std::string GetId() const;

[Wladimir Palant, 2015/01/22 10:47:20]

I think this method should be removed. The ID is an irrelevant implementation
detail. The client application has the Notification object to refer to
notifications.

[sergei, 2015/01/22 14:08:15]

removed

+    /**
+     * Retrieves the type of this notification.
+     * @return Type of this notification.
+     */
     NotificationType GetType() const;
+
     /**
-     * Localizes the texts of the supplied notification.
-     * @return the translated texts.
+     * Retrieves the title of this notification.
+     * @return Title of this notification.
      */
-    NotificationTexts GetTexts();

[Wladimir Palant, 2015/01/22 10:47:20]

An explicit method here shouldn't be necessary. The texts should rather be
retrieved when the notification is created and stored in protected properties.
There should then be public getters for these properties - the NotificationTexts
structure shouldn't be required.

[sergei, 2015/01/22 14:08:15]

fixed

[Felix Dahlke, 2015/01/23 21:27:50]

Missed this discussion before. I actually disagree, I don't see a real benefit
to how it was implemented now, I preferred how it was before. Reasons:

1. We can actually use the constructor to fully construct the Notification
object.
2. GetTexts().message is something Microsoft doesn't interfere with, as opposed
to GetMessage().
3. I really don't see how this could influence performance negatively. And even
if it would, we could cache the texts in a local static.

I think we should change this back again in a follow-up.

-    void SetTitle(const std::string& value);
-    void SetMessage(const std::string& value);
-    std::vector<std::string> GetUrlFilters() const;
-    void AddUrlFilter(const std::string& value);

[Wladimir Palant, 2015/01/22 10:47:20]

The four methods above should be removed - as discussed, notifications should be
read-only. The client application should never create its own notifications.

[sergei, 2015/01/22 14:08:15]

fixed

+    const std::string& GetTitle() const;
+
+    /**
+     * Retrieves the message of this notification.
+     * @return Message of this notification.
+     */
+    const std::string& GetMessageString() const;
+
+    /**
+     * Marks this notification as shown. It is only relevant for question
+     * notifications. Other notifications are marked automatically.
+     */
     void MarkAsShown();
+  private:
+    std::string title;
+    std::string message;
+    NotificationType type;
   };
+  typedef std::tr1::shared_ptr<Notification> NotificationPtr;
 }
 
 #endif