Merge lp:~feng-kylin/unity/change-desktop-label into lp:unity

[handsome_feng]

added support for getting the distro name from /etc/os-release

 Add support for getting the distro name from /etc/os-release ,use _("%s Desktop") instead of _("Ubuntu Desktop").

[Anthony Wong (anthonywong)]

LGTM

review: Approve

[Marco Trevisan (Treviño) (3v1n0)]

Please:
 - move the whole function out from the constructor, inside another function
   in the anonymous namespace.
 - Make it return the string, and call it in the initialization list
   i.e.: , desktop_name_(get_current_desktop())
 - Get rid of the tabs and use only 2-spaces indentation
 - See the diff comments

Also, the way to get the string is not much safe, since the apexes couldn't be there, so I'd do something like:
 os_release_name = boost::erase_all_copy(temp.substr(temp.find_last_of('=')+1), "\"")

There's also another problem we've to handle btw: this will break the translations we currently have.
So, if this fix is meant to be backported to 14.04 as well, then it's probably better to return the _("Ubuntu Desktop") string if the os_release_name == "Ubuntu".
This is not the nicest thing in the Earth, but it won't break translations.

review: Needs Fixing

[handsome_feng (feng-kylin)]

thank you very much for your detailed advice !

在2014年09月15 19时39分，"Marco Trevisan (Treviño)"<[e-mail]>写道：

Review: Needs Fixing

Please:
 - move the whole function out from the constructor, inside another function
   in the anonymous namespace.
 - Make it return the string, and call it in the initialization list
   i.e.: , desktop_name_(get_current_desktop())
 - Get rid of the tabs and use only 2-spaces indentation
 - See the diff comments

Also, the way to get the string is not much safe, since the apexes couldn't be there, so I'd do something like:
 os_release_name = boost::erase_all_copy(temp.substr(temp.find_last_of('=')+1), "\"")

There's also another problem we've to handle btw: this will break the translations we currently have.
So, if this fix is meant to be backported to 14.04 as well, then it's probably better to return the _("Ubuntu Desktop") string if the os_release_name == "Ubuntu".
This is not the nicest thing in the Earth, but it won't break translations.

Diff comments:

> === modified file 'panel/PanelMenuView.cpp'
> --- panel/PanelMenuView.cpp 2014-04-02 21:42:44 +0000
> +++ panel/PanelMenuView.cpp 2014-09-15 07:06:26 +0000
> @@ -73,8 +73,24 @@
> , ignore_menu_visibility_(false)
> , integrated_menus_(decoration::Style::Get()->integrated_menus())
> , active_xid_(0)
> - , desktop_name_(_("Ubuntu Desktop"))
> {
> + std::ifstream fin("/etc/os-release");
> + std::string temp,os_release_name = _("Ubuntu");

Keep these string definitions in two different lines. Not sure "Ubuntu" has to be translated btw.

> + if (fin.is_open())
> + {
> + while( getline(fin,temp) )

get rid of the trailing spaces, add a space between "fin, temp").

> + {
> + if (temp.substr(0,4) == "NAME")
> + {
> + size_t i = temp.find_first_of("\"");

Better to use: size_t i = temp.find_first_of('"', 4)

> + size_t j = temp.find_last_of("\"");

Better to use: size_t j = temp.find_last_of('"', i+1)

> + os_release_name = temp.substr(i+1, j-i-1);
> + break;
> + }
> + }
> + fin.close();
> + }
> + desktop_name_ = g_strdup_printf(_("%s Desktop"), os_release_name.c_str());
>
> BamfWindow* active_win = bamf_matcher_get_active_window(matcher_);
> if (BAMF_IS_WINDOW(active_win))
>
> === modified file 'panel/PanelMenuView.h'
> --- panel/PanelMenuView.h 2014-04-02 21:42:44 +0000
> +++ panel/PanelMenuView.h 2014-09-15 07:06:26 +0000
> @@ -188,7 +188,7 @@
>
> Window active_xid_;
> nux::Geometry monitor_geo_;
> - const std::string desktop_name_;
> + std::string desktop_name_;

This is not needed if you do as suggested.

>
> glib::Signal<void, BamfMatcher*, BamfView*> view_opened_signal_;
> glib::Signal<void, BamfMatcher*, BamfView*> view_closed_signal_;
>

--
https://code.launchpad.net/~445865575-b/unity/change-desktop-label/+merge/234624
You are the owner of lp:~445865575-b/unity/change-desktop-label.

[Marco Trevisan (Treviño) (3v1n0)]

Looks better, just few more comments... :)

[handsome_feng (feng-kylin)]

I received a great benefit from your comments,thank you very much! ( ^_^ )

在2014年09月17 07时58分，"Marco Trevisan (Treviño)"<[e-mail]>写道：

Looks better, just few more comments... :)

Diff comments:

> === modified file 'panel/PanelMenuView.cpp'
> --- panel/PanelMenuView.cpp 2014-04-02 21:42:44 +0000
> +++ panel/PanelMenuView.cpp 2014-09-16 06:16:41 +0000
> @@ -20,6 +20,7 @@
>
> #include <Nux/Nux.h>
> #include <NuxCore/Logger.h>
> +#include <boost/algorithm/string/erase.hpp>
>
> #include "PanelMenuView.h"
> #include "unity-shared/AnimationUtils.h"
> @@ -53,6 +54,34 @@
> const std::string WINDOW_ACTIVATED_TIMEOUT = "window-activated-timeout";
> const std::string UPDATE_SHOW_NOW_TIMEOUT = "update-show-now-timeout";
> const std::string INTEGRATED_MENUS_DOUBLE_CLICK_TIMEOUT = "integrated-menus-double-click-timeout";
> +
> +std::string get_current_desktop()
> +{
> + std::ifstream fin("/etc/os-release");
> + std::string temp;
> + std::string os_release_name("Ubuntu");
> + std::string desktop_name;

The destkop_name string is not needed, you can just return _("Ubuntu Desktop"); or glib::String(...)

> +
> + if (fin.is_open())
> + {
> + while (getline(fin, temp))
> + {
> + if (temp.substr(0,4) == "NAME")
> + {
> + os_release_name = boost::erase_all_copy(temp.substr(temp.find_last_of('=')+1), "\"");
> + break;
> + }
> + }
> + fin.close();
> + }
> +
> + if (os_release_name == "Ubuntu")

Add a safety check:
  if (os_release_name.empty() || os_release_name == "Ubuntu")

> + desktop_name = _("Ubuntu Desktop");

Please, include a comment explaining why of this "hack"

> + else
> + desktop_name = g_strdup_printf(_("%s Desktop"), os_release_name.c_str());

There's a leak here, instead you should:
 desktop_name = glib::String(g_strdup_printf(_("%s Desktop"), os_release_name.c_str())).Str();

> +
> + return desktop_name;
> +}
> }
>
> PanelMenuView::PanelMenuView(menu::Manager::Ptr const& menus)
> @@ -73,9 +102,8 @@
> , ignore_menu_visibility_(false)
> , integrated_menus_(decoration::Style::Get()->integrated_menus())
> , active_xid_(0)
> - , desktop_name_(_("Ubuntu Desktop"))
> + , desktop_name_(get_current_desktop())
> {
> -
> BamfWindow* active_win = bamf_matcher_get_active_window(matcher_);
> if (BAMF_IS_WINDOW(active_win))
> active_xid_ = bamf_window_get_xid(active_win);
>

--
https://code.launchpad.net/~445865575-b/unity/change-desktop-label/+merge/234624
You are the owner of lp:~445865575-b/unity/change-desktop-label.

[Marco Trevisan (Treviño) (3v1n0)]

Looks good, thanks!

review: Approve

=== modified file 'panel/PanelMenuView.cpp'
--- panel/PanelMenuView.cpp	2014-04-02 21:42:44 +0000
+++ panel/PanelMenuView.cpp	2014-09-17 03:50:47 +0000
@@ -20,6 +20,7 @@
 
 #include <Nux/Nux.h>
 #include <NuxCore/Logger.h>
+#include <boost/algorithm/string/erase.hpp>
 
 #include "PanelMenuView.h"
 #include "unity-shared/AnimationUtils.h"
@@ -53,6 +54,32 @@
   const std::string WINDOW_ACTIVATED_TIMEOUT = "window-activated-timeout";
   const std::string UPDATE_SHOW_NOW_TIMEOUT = "update-show-now-timeout";
   const std::string INTEGRATED_MENUS_DOUBLE_CLICK_TIMEOUT = "integrated-menus-double-click-timeout";
+
+std::string get_current_desktop()
+{
+  std::ifstream fin("/etc/os-release");
+  std::string temp;
+  std::string os_release_name("Ubuntu");
+
+  if (fin.is_open())
+  {
+    while (getline(fin, temp))
+    {
+      if (temp.substr(0,4) == "NAME")
+      {
+        os_release_name = boost::erase_all_copy(temp.substr(temp.find_last_of('=')+1), "\"");
+        break;
+      }
+    }
+    fin.close();
+  }
+
+  //this is done to avoid breaking translation before 14.10.
+  if (os_release_name.empty() || os_release_name == "Ubuntu")
+    return _("Ubuntu Desktop");
+  else
+    return glib::String(g_strdup_printf(_("%s Desktop"), os_release_name.c_str())).Str();
+}
 }
 
 PanelMenuView::PanelMenuView(menu::Manager::Ptr const& menus)
@@ -73,9 +100,8 @@
   , ignore_menu_visibility_(false)
   , integrated_menus_(decoration::Style::Get()->integrated_menus())
   , active_xid_(0)
-  , desktop_name_(_("Ubuntu Desktop"))
+  , desktop_name_(get_current_desktop())
 {
-
   BamfWindow* active_win = bamf_matcher_get_active_window(matcher_);
   if (BAMF_IS_WINDOW(active_win))
     active_xid_ = bamf_window_get_xid(active_win);