Merge lp:~knitzsche/telegram-app/recent-keyword-aggreagation into lp:telegram-app/app

[Kyle Nitzsche]

this vivid branch supports keyword aggregation (introduced in vivid) of telegram scope by "recent" keyword using the components and result fields expected by "recent"" aggregation. The one most recent result is returned when being aggregated by "recent" keyword.

[Kyle Nitzsche (knitzsche)]

by the way, generated (c)make files are not ignored in bzr and thus (unexpectedly to me) they show up here in the diff. The only files that really matter to this MR's functionality are scope.ini.in, query.cpp and query.h. cheers

[Roberto Mier Escandon (rmescandon)]

Why scope/po/Makefile and scope/po/cmake_install.cmake are in the repository. Should they be ignored instead?
In any case, they have abs path to your local folders :S

review: Needs Fixing

[Michał Karnicki (karni)]

I am aware that current scope source may not be super clean, but let's try to make it better.

I would have the following suggestions:
0) what Roberto said - it's less about the local paths present, more about some noise in this MP's diff
1) rename md_ to mMetaData (to follow rest of member fields, like mScopeDir)
2) stick to one convention - camel case, I see recentMessages, agged_by_recent and totalunread:
+ auto recentMessages = reply->register_category("telegram_recent", "Telegram", "", catRecentRendr);
[...]
+ if (!agged_by_recent && totalunread > 0 && search_query.isEmpty()) {
Yes I'm aware two of these where already there in this form, let's just focus on making it better. Feel free to also s/totalunread/totalUnread
3) please open blocks of code with brace on same line, and feel free to put lone 'break;'s on same line as if conditions, if you want - if not, please wrap in { }
4) instead of:
    for (unsigned int i = 0; i < results.size(); i++)
    {
        if (agged_by_recent)
        {
        // huge block here
        break;
        reply->push(results[i]);
    }
I would suggest the following, which should improve readability:
    if (agged_by_recent) {
        // do stuff here
        return;
    }
    for (...) {
        reply->push(results[i]);
    }

I think this would be solid. The new logic looks sane.

review: Needs Fixing

[Roberto Mier Escandon (rmescandon)]

Just a comment for Karni:
C++ 11 uses this_annotation and Qt uses thisOther ... so... it's not easy agree on one of them since everybody is right regardless of what his opinion is.

In fw we have the next "convention" for everybody to be happy:

- all c++ 11 related code uses underscore annotation
- all Qt related code uses camel case

so that you can have instructions like:

const std::atype& a_var = "a value";
const QVariant &another = QVariant("another");
QString myString = QString::fromStdString(one_object.calling_any_c_eleven_method(a_var, another));

besides, in scopes world the usual way you find code is in c++ 11 way, so that annotation can't be skipped

[Michał Karnicki (karni)]

Thank you for your feedback, Roberto. To be clear, I'm not in favor of one of those. I'm simply in favor of consistency :) I don't mind if the rest is converted to use underscore notation, as long as we try to be consistent. (Regardless, for instance, the md_ is neither descriptive nor consistent with any other member in that source. Perhaps meta_data would be better, if all other variables/members weren't camel case.)

[Kyle Nitzsche (knitzsche)]

Thanks guys.
New branch about to be posted. Responses to comments above:
0) I have bzr ignored the po/ cmake/make files. (seems odd that I should need to do this since I have not modified any build scripts or introduced new terms for l10n... but c'est la vie :)
1) have changed md_ to mMetadata (not mMetaData since the api uses a lower case 'd'.
2) have changed agged_by_recent to aggedByRecent
3) & 4) I open blocks on the same line
cheers - Kyle

[lp:~knitzsche/telegram-app/recent-keyword-aggreagation]

109. By Kyle Nitzsche on 2015-06-03

modified code conventions to fit this project per MR request

[Kyle Nitzsche (knitzsche)]

hang on, the Makefile and cmake files are still present

[lp:~knitzsche/telegram-app/recent-keyword-aggreagation]

110. By Kyle Nitzsche on 2015-06-03

bzr ignore scope/po/Makefile & scope/po/cmake_install.cmake

[Kyle Nitzsche (knitzsche)]

OK, I removed and ignored the scope/po/ make files

[Roberto Mier Escandon (rmescandon)]

lgtm (though not tested)

review: Approve

[Kyle Nitzsche (knitzsche)]

Roberto, can you top approve pls? Looks like karni is on holiday.

=== modified file '.bzrignore'
--- .bzrignore	2015-05-11 08:49:51 +0000
+++ .bzrignore	2015-06-03 13:43:06 +0000
@@ -13,8 +13,9 @@
 third_party
 telegram
 telegram_automoc.cpp
-
 Makefile
+po/Makefile
+po/cmake_install.cmake
 CMakeLists.txt.user
 CMakeFiles
 CMakeCache.txt
@@ -34,3 +35,5 @@
 scope/src/com.ubuntu.telegram_sctelegram_automoc.cpp
 .ycm_extra_conf.py
 scope/po/built_mos
+scope/po/Makefile
+scope/po/cmake_install.cmake

=== modified file 'po/com.ubuntu.telegram.pot'
--- po/com.ubuntu.telegram.pot	2015-06-01 15:01:21 +0000
+++ po/com.ubuntu.telegram.pot	2015-06-03 13:43:06 +0000
@@ -8,7 +8,7 @@
 msgstr ""
 "Project-Id-Version: \n"
 "Report-Msgid-Bugs-To: \n"
-"POT-Creation-Date: 2015-06-01 17:00+0200\n"
+"POT-Creation-Date: 2015-06-03 09:40-0400\n"
 "PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n"
 "Last-Translator: FULL NAME <EMAIL@ADDRESS>\n"
 "Language-Team: LANGUAGE <[e-mail]>\n"

=== modified file 'scope/data/com.ubuntu.telegram_sctelegram.ini'
--- scope/data/com.ubuntu.telegram_sctelegram.ini	2015-06-01 15:01:21 +0000
+++ scope/data/com.ubuntu.telegram_sctelegram.ini	2015-06-03 13:43:06 +0000
@@ -68,6 +68,7 @@
 SearchHint[uk]=Шукати повідомлення
 SearchHint[zh_TW]=搜尋訊息
 HotKey=tasks.HotKey
+Keywords=recent
 [Appearance]
 background=color:///#497495
 PageHeader.Logo=./images/logo.png

=== modified file 'scope/data/scope.ini.in'
--- scope/data/scope.ini.in	2014-12-02 12:35:48 +0000
+++ scope/data/scope.ini.in	2015-06-03 13:43:06 +0000
@@ -6,6 +6,7 @@
 Icon=./images/telegram.png
 _SearchHint=Search for a message
 HotKey=tasks.HotKey
+Keywords=recent
 [Appearance]
 background=color:///#497495
 PageHeader.Logo=./images/logo.png

=== removed file 'scope/po/Makefile'
--- scope/po/Makefile	2015-04-15 08:59:54 +0000
+++ scope/po/Makefile	1970-01-01 00:00:00 +0000
@@ -1,167 +0,0 @@
-# CMAKE generated file: DO NOT EDIT!
-# Generated by "Unix Makefiles" Generator, CMake Version 2.8
-
-# Default target executed when no arguments are given to make.
-default_target: all
-.PHONY : default_target
-
-#=============================================================================
-# Special targets provided by cmake.
-
-# Disable implicit rules so canonical targets will work.
-.SUFFIXES:
-
-# Remove some rules from gmake that .SUFFIXES does not remove.
-SUFFIXES =
-
-.SUFFIXES: .hpux_make_needs_suffix_list
-
-# Suppress display of executed commands.
-$(VERBOSE).SILENT:
-
-# A target that is always out of date.
-cmake_force:
-.PHONY : cmake_force
-
-#=============================================================================
-# Set environment variables for the build.
-
-# The shell in which to execute make rules.
-SHELL = /bin/sh
-
-# The CMake executable.
-CMAKE_COMMAND = /usr/bin/cmake
-
-# The command to remove a file.
-RM = /usr/bin/cmake -E remove -f
-
-# Escaping for special characters.
-EQUALS = =
-
-# The top-level source directory on which CMake was run.
-CMAKE_SOURCE_DIR = /home/karni/src/canonical/telegram/app
-
-# The top-level build directory on which CMake was run.
-CMAKE_BINARY_DIR = /home/karni/src/canonical/telegram/app
-
-#=============================================================================
-# Targets provided globally by CMake.
-
-# Special rule for the target edit_cache
-edit_cache:
-	@$(CMAKE_COMMAND) -E cmake_echo_color --switch=$(COLOR) --cyan "Running interactive CMake command-line interface..."
-	/usr/bin/cmake -i .
-.PHONY : edit_cache
-
-# Special rule for the target edit_cache
-edit_cache/fast: edit_cache
-.PHONY : edit_cache/fast
-
-# Special rule for the target install
-install: preinstall
-	@$(CMAKE_COMMAND) -E cmake_echo_color --switch=$(COLOR) --cyan "Install the project..."
-	/usr/bin/cmake -P cmake_install.cmake
-.PHONY : install
-
-# Special rule for the target install
-install/fast: preinstall/fast
-	@$(CMAKE_COMMAND) -E cmake_echo_color --switch=$(COLOR) --cyan "Install the project..."
-	/usr/bin/cmake -P cmake_install.cmake
-.PHONY : install/fast
-
-# Special rule for the target install/local
-install/local: preinstall
-	@$(CMAKE_COMMAND) -E cmake_echo_color --switch=$(COLOR) --cyan "Installing only the local directory..."
-	/usr/bin/cmake -DCMAKE_INSTALL_LOCAL_ONLY=1 -P cmake_install.cmake
-.PHONY : install/local
-
-# Special rule for the target install/local
-install/local/fast: install/local
-.PHONY : install/local/fast
-
-# Special rule for the target install/strip
-install/strip: preinstall
-	@$(CMAKE_COMMAND) -E cmake_echo_color --switch=$(COLOR) --cyan "Installing the project stripped..."
-	/usr/bin/cmake -DCMAKE_INSTALL_DO_STRIP=1 -P cmake_install.cmake
-.PHONY : install/strip
-
-# Special rule for the target install/strip
-install/strip/fast: install/strip
-.PHONY : install/strip/fast
-
-# Special rule for the target list_install_components
-list_install_components:
-	@$(CMAKE_COMMAND) -E cmake_echo_color --switch=$(COLOR) --cyan "Available install components are: \"Unspecified\""
-.PHONY : list_install_components
-
-# Special rule for the target list_install_components
-list_install_components/fast: list_install_components
-.PHONY : list_install_components/fast
-
-# Special rule for the target rebuild_cache
-rebuild_cache:
-	@$(CMAKE_COMMAND) -E cmake_echo_color --switch=$(COLOR) --cyan "Running CMake to regenerate build system..."
-	/usr/bin/cmake -H$(CMAKE_SOURCE_DIR) -B$(CMAKE_BINARY_DIR)
-.PHONY : rebuild_cache
-
-# Special rule for the target rebuild_cache
-rebuild_cache/fast: rebuild_cache
-.PHONY : rebuild_cache/fast
-
-# The main all target
-all: cmake_check_build_system
-	cd /home/karni/src/canonical/telegram/app && $(CMAKE_COMMAND) -E cmake_progress_start /home/karni/src/canonical/telegram/app/CMakeFiles /home/karni/src/canonical/telegram/app/scope/po/CMakeFiles/progress.marks
-	cd /home/karni/src/canonical/telegram/app && $(MAKE) -f CMakeFiles/Makefile2 scope/po/all
-	$(CMAKE_COMMAND) -E cmake_progress_start /home/karni/src/canonical/telegram/app/CMakeFiles 0
-.PHONY : all
-
-# The main clean target
-clean:
-	cd /home/karni/src/canonical/telegram/app && $(MAKE) -f CMakeFiles/Makefile2 scope/po/clean
-.PHONY : clean
-
-# The main clean target
-clean/fast: clean
-.PHONY : clean/fast
-
-# Prepare targets for installation.
-preinstall: all
-	cd /home/karni/src/canonical/telegram/app && $(MAKE) -f CMakeFiles/Makefile2 scope/po/preinstall
-.PHONY : preinstall
-
-# Prepare targets for installation.
-preinstall/fast:
-	cd /home/karni/src/canonical/telegram/app && $(MAKE) -f CMakeFiles/Makefile2 scope/po/preinstall
-.PHONY : preinstall/fast
-
-# clear depends
-depend:
-	cd /home/karni/src/canonical/telegram/app && $(CMAKE_COMMAND) -H$(CMAKE_SOURCE_DIR) -B$(CMAKE_BINARY_DIR) --check-build-system CMakeFiles/Makefile.cmake 1
-.PHONY : depend
-
-# Help Target
-help:
-	@echo "The following are some of the valid targets for this Makefile:"
-	@echo "... all (the default if no target is provided)"
-	@echo "... clean"
-	@echo "... depend"
-	@echo "... edit_cache"
-	@echo "... install"
-	@echo "... install/local"
-	@echo "... install/strip"
-	@echo "... list_install_components"
-	@echo "... rebuild_cache"
-.PHONY : help
-
-
-
-#=============================================================================
-# Special targets to cleanup operation of make.
-
-# Special rule to run CMake to check the build system integrity.
-# No rule that depends on this can have commands that come from listfiles
-# because they might be regenerated.
-cmake_check_build_system:
-	cd /home/karni/src/canonical/telegram/app && $(CMAKE_COMMAND) -H$(CMAKE_SOURCE_DIR) -B$(CMAKE_BINARY_DIR) --check-build-system CMakeFiles/Makefile.cmake 0
-.PHONY : cmake_check_build_system
-

=== removed file 'scope/po/cmake_install.cmake'
--- scope/po/cmake_install.cmake	2015-04-15 08:59:54 +0000
+++ scope/po/cmake_install.cmake	1970-01-01 00:00:00 +0000
@@ -1,46 +0,0 @@
-# Install script for directory: /home/karni/src/canonical/telegram/app/scope/po
-
-# Set the install prefix
-IF(NOT DEFINED CMAKE_INSTALL_PREFIX)
-  SET(CMAKE_INSTALL_PREFIX "/")
-ENDIF(NOT DEFINED CMAKE_INSTALL_PREFIX)
-STRING(REGEX REPLACE "/$" "" CMAKE_INSTALL_PREFIX "${CMAKE_INSTALL_PREFIX}")
-
-# Set the install configuration name.
-IF(NOT DEFINED CMAKE_INSTALL_CONFIG_NAME)
-  IF(BUILD_TYPE)
-    STRING(REGEX REPLACE "^[^A-Za-z0-9_]+" ""
-           CMAKE_INSTALL_CONFIG_NAME "${BUILD_TYPE}")
-  ELSE(BUILD_TYPE)
-    SET(CMAKE_INSTALL_CONFIG_NAME "")
-  ENDIF(BUILD_TYPE)
-  MESSAGE(STATUS "Install configuration: \"${CMAKE_INSTALL_CONFIG_NAME}\"")
-ENDIF(NOT DEFINED CMAKE_INSTALL_CONFIG_NAME)
-
-# Set the component getting installed.
-IF(NOT CMAKE_INSTALL_COMPONENT)
-  IF(COMPONENT)
-    MESSAGE(STATUS "Install component: \"${COMPONENT}\"")
-    SET(CMAKE_INSTALL_COMPONENT "${COMPONENT}")
-  ELSE(COMPONENT)
-    SET(CMAKE_INSTALL_COMPONENT)
-  ENDIF(COMPONENT)
-ENDIF(NOT CMAKE_INSTALL_COMPONENT)
-
-# Install shared libraries without execute permission?
-IF(NOT DEFINED CMAKE_INSTALL_SO_NO_EXE)
-  SET(CMAKE_INSTALL_SO_NO_EXE "1")
-ENDIF(NOT DEFINED CMAKE_INSTALL_SO_NO_EXE)
-
-IF(NOT CMAKE_INSTALL_COMPONENT OR "${CMAKE_INSTALL_COMPONENT}" STREQUAL "Unspecified")
-  list(APPEND CMAKE_ABSOLUTE_DESTINATION_FILES
-   "/sctelegram/locale")
-  IF (CMAKE_WARN_ON_ABSOLUTE_INSTALL_DESTINATION)
-    message(WARNING "ABSOLUTE path INSTALL DESTINATION : ${CMAKE_ABSOLUTE_DESTINATION_FILES}")
-  ENDIF (CMAKE_WARN_ON_ABSOLUTE_INSTALL_DESTINATION)
-  IF (CMAKE_ERROR_ON_ABSOLUTE_INSTALL_DESTINATION)
-    message(FATAL_ERROR "ABSOLUTE path INSTALL DESTINATION forbidden (by caller): ${CMAKE_ABSOLUTE_DESTINATION_FILES}")
-  ENDIF (CMAKE_ERROR_ON_ABSOLUTE_INSTALL_DESTINATION)
-FILE(INSTALL DESTINATION "/sctelegram" TYPE DIRECTORY FILES "/home/karni/src/canonical/telegram/app/scope/po/built_mos/locale")
-ENDIF(NOT CMAKE_INSTALL_COMPONENT OR "${CMAKE_INSTALL_COMPONENT}" STREQUAL "Unspecified")
-

=== modified file 'scope/src/query.cpp'
--- scope/src/query.cpp	2015-02-11 12:38:39 +0000
+++ scope/src/query.cpp	2015-06-03 13:43:06 +0000
@@ -3,6 +3,7 @@
 #include <unity/scopes/CategoryRenderer.h>
 #include <unity/scopes/QueryBase.h>
 #include <unity/scopes/SearchReply.h>
+#include <unity/scopes/VariantBuilder.h>
 
 #include <QDebug>
 #include <QUrl>
@@ -21,6 +22,25 @@
 
 namespace
 {
+
+static const std::string RECENT = R"(
+{
+    "schema-version": 1,
+    "template":
+    {
+        "category-layout": "grid",
+        "card-layout": "horizontal",
+        "card-size": "large"
+    },
+    "components":
+    {
+        "title": "title",
+        "attributes": {"field": "attributes", "max-count": 4},
+        "mascot": "mascot"
+    }
+}
+)";
+
 const static std::string UNREAD_TEMPLATE =
         R"(
 {
@@ -154,7 +174,7 @@
 }
 
 TelegramQuery::TelegramQuery(CannedQuery const& query, SearchMetadata const& metadata, QString const& scopeDir) :
-    SearchQueryBase(query, metadata), mScopeDir(scopeDir)
+    SearchQueryBase(query, metadata), mMetadata(metadata), mScopeDir(scopeDir)
 {
  setlocale(LC_ALL, "");
  textdomain(GETTEXT_DOMAIN.toStdString().c_str());
@@ -452,17 +472,26 @@
 
 void TelegramQuery::run(unity::scopes::SearchReplyProxy const& reply)
 {
+    bool aggedByRecent = false;
     unsigned int recentMax = 10;
+    if (mMetadata.is_aggregated()){
+        auto keywords = mMetadata.aggregated_keywords();
+        if (keywords.find("recent") != keywords.end()){
+            aggedByRecent = true;
+        }
+    }
     //Tell gettext where to look for textdomain files
     QString tdir = QString("%1/locale/").arg(mScopeDir);
     QString basedir = QString(bindtextdomain(GETTEXT_DOMAIN.toStdString().c_str(), tdir.toStdString().c_str()));
     QString search_query = QString::fromStdString(query().query_string());
 
     CategoryRenderer catReadRendr(search_query.isEmpty()?READ_TEMPLATE:SEARCH_MESS_TEMPLATE);
+    CategoryRenderer catRecentRendr(RECENT);
     CategoryRenderer catUnreadRendr(UNREAD_TEMPLATE);
     CategoryRenderer carrRendr(search_query.isEmpty()?CARR_TEMPLATE:CONTACTS_SEARCH_TEMPLATE);
     CategoryRenderer photoRendr(PHOTO_TEMPLATE);
 
+    auto recentMessages = reply->register_category("telegram_recent", "Telegram", "", catRecentRendr);
     auto readMessages = reply->register_category("read", _("Recent Messages"), "", catReadRendr);
     auto buddiesCat = reply->register_category("buddies", _("People You Talk To"), "", carrRendr);
     auto photoCat = reply->register_category("photos", _("Received Images"), "", photoRendr);;
@@ -485,7 +514,7 @@
 
     auto totalunread = getUnreadMessages(myTelegram);
 
-    if (totalunread > 0 && search_query.isEmpty()) {
+    if (!aggedByRecent && totalunread > 0 && search_query.isEmpty()) {
         QString title_unread = _("Unread Messages: %1");
         title_unread = title_unread.arg(totalunread);
         auto unReadMessages = reply->register_category("unread",title_unread.toStdString(), "", catUnreadRendr);
@@ -503,24 +532,37 @@
     for (unsigned int i = 0 ; i < myTelegram.size(); i++){
         if (myTelegram[i].out || unReadIds.find(myTelegram[i].id) != unReadIds.end())
            continue;
+        if (aggedByRecent && results.size() > 0)
+            break;// recent keyword aggregation only wants one message
         recieveMsgs++;
         myBuddies[myTelegram[i].from.id] = myTelegram[i].from;
         if (recieveMsgs <= recentMax && search_query.isEmpty()){
             auto res = messageToResult(readMessages,myTelegram[i],false);
             results.push_back(res);
+            if (aggedByRecent){
+                results.clear();// we want only the last item in the vector
+                results.push_back(res);
+                break;// recent keyword aggregation only wants one message
+            }
         }
         if (!search_query.isEmpty() && myTelegram[i].text.contains(search_query,Qt::CaseInsensitive)){
             auto res = messageToResult(searchReadMessages,myTelegram[i],false);
             results.push_back(res);
+            if (aggedByRecent)
+                break;// recent keyword aggregation only wants one message
         }
         if (search_query.isEmpty() &&
                 (myTelegram[i].mediaType == MessageMediaType::typeMessageMediaPhoto ||
                  myTelegram[i].mediaType == MessageMediaType::typeMessageMediaVideo)) {
             auto res = messageToResult(photoCat,myTelegram[i],true);
             results.push_back(res);
+            if (aggedByRecent)
+                break;// recent keyword aggregation only wants one message
         }
     }
     for (auto& mycontact: myBuddies) {
+        if (aggedByRecent)
+            break;// recent keyword aggregation only wants one message
          if ((mycontact.second.firstName.isEmpty() && mycontact.second.lastName.isEmpty()) ||
                  mycontact.second.id == mOurId) {
              continue;
@@ -536,6 +578,33 @@
              results.insert(results.begin(),res);
          }
     }
-    for (unsigned int i = 0; i < results.size(); i++)
+    if (aggedByRecent){
+        auto res = results[0];
+        // open telegram scope instead of preview:
+        res.set_uri("scope://com.ubuntu.telegram_sctelegram");
+        // set up result as recent aggregation expects it
+        std::string title = res["title"].get_string();
+        res["title"] = res["from"];
+        unity::scopes::VariantBuilder attrs;
+        attrs.add_tuple({
+            {"value", unity::scopes::Variant(title)}
+        });
+        attrs.add_tuple({
+            {"value", unity::scopes::Variant("")}
+        });
+        attrs.add_tuple({
+            {"value", unity::scopes::Variant(res["date"])}
+        });
+        attrs.add_tuple({
+            {"value", unity::scopes::Variant("")}
+        });
+        res["attributes"] = attrs.end();
+        std::string icon = QString("file://%1/images/telegram.png").arg(mScopeDir).toStdString();
+        res["mascot"] = icon;
+        reply->push(res);
+        return;
+    }
+    for (unsigned int i = 0; i < results.size(); i++){
         reply->push(results[i]);
+    }
 }

=== modified file 'scope/src/query.h'
--- scope/src/query.h	2014-12-05 14:52:19 +0000
+++ scope/src/query.h	2015-06-03 13:43:06 +0000
@@ -2,8 +2,10 @@
 #define DEMOQUERY_H
 
 #include <unity/scopes/SearchQueryBase.h>
+#include <unity/scopes/SearchMetadata.h>
 #include <unity/scopes/ReplyProxyFwd.h>
 #include <unity/scopes/CategorisedResult.h>
+#include <unity/scopes/CategorisedResult.h>
 #include <QtSql>
 #include <QSqlDatabase>
 #include <QFileInfo>
@@ -55,6 +57,7 @@
     unity::scopes::CategorisedResult messageToResult(unity::scopes::Category::SCPtr category, TelegramMessage message,bool useAttachment);
     unity::scopes::CategorisedResult contactToResult(unity::scopes::Category::SCPtr category, TelegramContact contact);
     int getUnreadMessages(std::vector<TelegramMessage> & unreadMessages);
+    unity::scopes::SearchMetadata mMetadata;
 };
 
 #endif