Merge lp:~andreagrandi/unity-2d/main into lp:unity-2d/3.0

[Andrea Grandi]

I've fixed bug #715611. Now when you delete something and you're using Unity-2D the Trash icon is updated as expected: if there are some files in the Trash the icon is the full Trash one, else is the empty one.

[Ugo Riboni (uriboni)]

Hi, thanks for the patch :)
I tested it and it works well.

Here's a few things that need to be changed in the code (some are bugs, some are just style issues).

* You are never deleting the monitor that you create with
monitor = g_file_monitor_directory (file, G_FILE_MONITOR_NONE, NULL, NULL);
You should probably save it to a member variable and call g_object_unref on it in the Trash destructor.

* Can you please change the name of the function initTrashIcon to updateTrashIcon ? (It's updating the icon many times, not initializing it only once)

* You don't need to pass to fileChanged all the same arguments as fileChangedProxy. In fact you can just have fileChanged without any arguments and check if the event_type is the right type in fileChangedProxy.

* We have some coding guidelines in place and we can only accept software that conform to these. You can find the guidelines in the CODING file in the root of the lp:unity-2d repo.
For example you need to change function names like start_monitoring_trash with startMonitoringTrash, names of member variables to start with "m_" (like m_iconName instead of iconName), names of local variables like _this to something like currentInstance or something similar.
Please have a look at the guidelines for other coding standards we follow (like how to put parenthesis after if/else and so on).

* Finally, if you did not submit it already, you have to take care of accepting the Canonical contributor agreement. You can find instructions at http://www.canonical.com/contributors . Hopefully you are ok with that.

Thanks !

review: Needs Fixing

[lp:~andreagrandi/unity-2d/main]

584. By Andrea Grandi on 2011-06-06

Fix bug 715611 and some refactory

585. By Andrea Grandi on 2011-06-06

Fix some typo

[Andrea Grandi (andreagrandi)]

Hi,

> Hi, thanks for the patch :)
> I tested it and it works well.

nice to hear :)

> * You are never deleting the monitor that you create with
> monitor = g_file_monitor_directory (file, G_FILE_MONITOR_NONE, NULL, NULL);
> You should probably save it to a member variable and call g_object_unref on it
> in the Trash destructor.

fixed

> * Can you please change the name of the function initTrashIcon to
> updateTrashIcon ? (It's updating the icon many times, not initializing it only
> once)

fixed

> * You don't need to pass to fileChanged all the same arguments as
> fileChangedProxy. In fact you can just have fileChanged without any arguments
> and check if the event_type is the right type in fileChangedProxy.

fixed

> * We have some coding guidelines in place and we can only accept software that
> conform to these. You can find the guidelines in the CODING file in the root
> of the lp:unity-2d repo.

read

> For example you need to change function names like start_monitoring_trash with
> startMonitoringTrash, names of member variables to start with "m_" (like
> m_iconName instead of iconName), names of local variables like _this to
> something like currentInstance or something similar.
> Please have a look at the guidelines for other coding standards we follow
> (like how to put parenthesis after if/else and so on).

fixed

> * Finally, if you did not submit it already, you have to take care of
> accepting the Canonical contributor agreement. You can find instructions at
> http://www.canonical.com/contributors . Hopefully you are ok with that.

done!

Please let me know if the new patch is ok :)

[Ugo Riboni (uriboni)]

All fixes are good, thanks for taking the time to do them.
There's just a few things missing:

68 + Trash* _this = static_cast<Trash*>(data);
69 +
70 + switch (event_type)
71 + {
72 + case (G_FILE_MONITOR_EVENT_DELETED || G_FILE_MONITOR_EVENT_CREATED):
73 + return _this->fileChanged();
74 + break;
75 + default: ;
76 + }

Please rename "_this" to "instance", and instead of the switch/case use a simpler and shorter if statement.

After you fix that, I will approve it and it will be merged into the trunk.

review: Needs Fixing

[Ugo Riboni (uriboni)]

Or maybe even better:

if (event_type == G_FILE_MONITOR_EVENT_DELETED || event_type == G_FILE_MONITOR_EVENT_CREATED) {
    (static_cast<Trash*>(data))->fileChanged();
}

[lp:~andreagrandi/unity-2d/main]

586. By Andrea Grandi on 2011-06-07

Refactory some code in Trash

[Andrea Grandi (andreagrandi)]

> All fixes are good, thanks for taking the time to do them.
> There's just a few things missing:
>
> 68 + Trash* _this = static_cast<Trash*>(data);
> 69 +
> 70 + switch (event_type)
> 71 + {
> 72 + case (G_FILE_MONITOR_EVENT_DELETED ||
> G_FILE_MONITOR_EVENT_CREATED):
> 73 + return _this->fileChanged();
> 74 + break;
> 75 + default: ;
> 76 + }
>
> Please rename "_this" to "instance", and instead of the switch/case use a
> simpler and shorter if statement.
>
> After you fix that, I will approve it and it will be merged into the trunk.

done, tested, committed! :)

[Ugo Riboni (uriboni)]

68 + if (event_type == G_FILE_MONITOR_EVENT_DELETED || event_type == G_FILE_MONITOR_EVENT_CREATED) {
69 + (static_cast<Trash*>(data))->fileChanged(); }

The closing brace should be on the next line(yes, i'm picky. sorry about that ;))

review: Needs Fixing

[lp:~andreagrandi/unity-2d/main]

587. By Andrea Grandi on 2011-06-07

Fix a closing brace

[Andrea Grandi (andreagrandi)]

> 68 + if (event_type == G_FILE_MONITOR_EVENT_DELETED || event_type ==
> G_FILE_MONITOR_EVENT_CREATED) {
> 69 + (static_cast<Trash*>(data))->fileChanged(); }
>
> The closing brace should be on the next line(yes, i'm picky. sorry about that
> ;))

fixed :P

[Ugo Riboni (uriboni)]

Everything is ok. Tested again and it's all working.
Approved and will be merged soon.
Thanks for all your work :)

review: Approve

=== modified file 'launcher/UnityApplications/trash.cpp'
--- launcher/UnityApplications/trash.cpp	2011-05-19 13:16:52 +0000
+++ launcher/UnityApplications/trash.cpp	2011-06-07 09:36:36 +0000
@@ -39,6 +39,8 @@
 {
     m_trash = g_file_new_for_uri(TRASH_URI);
     setShortcutKey(Qt::Key_T);
+    updateTrashIcon();
+    startMonitoringTrash();
 }
 
 Trash::Trash(const Trash& other)
@@ -47,6 +49,7 @@
 
 Trash::~Trash()
 {
+    g_object_unref(m_monitor);
     g_object_unref(m_trash);
 }
 
@@ -125,7 +128,7 @@
 QString
 Trash::icon() const
 {
-    return "unity-icon-theme/user-trash";
+    return m_iconName;
 }
 
 bool
@@ -167,7 +170,7 @@
 void
 Trash::empty() const
 {
-    recursive_delete(m_trash);
+    recursiveDelete(m_trash);
 }
 
 int
@@ -192,7 +195,7 @@
 }
 
 void
-Trash::recursive_delete(GFile* dir)
+Trash::recursiveDelete(GFile* dir)
 {
     GError* error = NULL;
     QString attributes;
@@ -215,7 +218,7 @@
     while ((info = g_file_enumerator_next_file(children, NULL, &error)) != NULL) {
         GFile* child = g_file_get_child(dir, g_file_info_get_name(info));
         if (g_file_info_get_file_type(info) == G_FILE_TYPE_DIRECTORY) {
-            recursive_delete(child);
+            recursiveDelete(child);
         }
 
         /* If passed a GError* as third parameter, g_file_delete incorrectly
@@ -326,3 +329,42 @@
     return QVariant::fromValue(m_trash);
 }
 
+void
+Trash::fileChangedProxy(GFileMonitor      *file_monitor,
+              GFile             *child,
+              GFile             *other_file,
+              GFileMonitorEvent  event_type,
+              gpointer           data)
+{
+    if (event_type == G_FILE_MONITOR_EVENT_DELETED || event_type == G_FILE_MONITOR_EVENT_CREATED) {
+       (static_cast<Trash*>(data))->fileChanged();
+    }
+}
+
+void
+Trash::fileChanged()
+{
+    updateTrashIcon();
+    emit iconChanged(icon());
+}
+
+void
+Trash::startMonitoringTrash(void)
+{
+    GFile *file;
+
+    file = g_file_new_for_uri ("trash://");
+    m_monitor = g_file_monitor_directory (file, G_FILE_MONITOR_NONE, NULL, NULL);
+    g_object_unref(file);
+
+    g_signal_connect(m_monitor, "changed", G_CALLBACK(Trash::fileChangedProxy), this);
+}
+
+void
+Trash::updateTrashIcon(void)
+{
+    if(count() == 0) {
+        m_iconName = "unity-icon-theme/user-trash"; }
+    else {
+        m_iconName = "unity-icon-theme/user-trash-full"; }
+}

=== modified file 'launcher/UnityApplications/trash.h'
--- launcher/UnityApplications/trash.h	2011-05-19 13:16:52 +0000
+++ launcher/UnityApplications/trash.h	2011-06-07 09:36:36 +0000
@@ -65,6 +65,9 @@
 private Q_SLOTS:
     void onEmptyTriggered();
 
+    static void fileChangedProxy(GFileMonitor *file_monitor, GFile *child, GFile *other_file, GFileMonitorEvent event_type, gpointer user_data);
+    void fileChanged();
+
 private:
     void open() const;
     void empty() const;
@@ -73,9 +76,13 @@
     QList<WnckWindow*> trashWindows() const;
     bool isTrashWindow(WnckWindow* window) const;
 
-    static void recursive_delete(GFile* dir);
+    static void recursiveDelete(GFile* dir);
+    void startMonitoringTrash();
+    void updateTrashIcon();
 
+    QString m_iconName;
     GFile* m_trash;
+    GFileMonitor* m_monitor;
 };
 
 Q_DECLARE_METATYPE(Trash*)