Code review comment for lp://qastaging/~gordallott/unity/dash-legal-link

Revision history for this message
Marco Trevisan (TreviƱo) (3v1n0) wrote :

Looks good and works well...

29 + glib::String cachedir(g_strdup(g_get_user_cache_dir()));

No need to strdup it, just assign that to std::string... It should be never NULL, in case just check for that.

30 + legal_seen_file_path_ = cachedir.Str() + "/unitydashlegalseen";

Since we have an unity .cache dir, I'd just add there... i.e. cachedir/unity/dashlegalseen

68 + QueueRelayout();
69 + QueueDraw();

The first is enough, as does also redraw.

64 + info_icon_->SetVisible(info_previously_shown_);
65 + legal_->SetVisible(!info_previously_shown_);
67 + DoOpenLegalise();

Wouldn't be better to open the legalise (and close the dash) and then switch the icon visibility?

95 + std::string legal_file_path = "file://";
96 + legal_file_path.append(PKGDATADIR);
97 + legal_file_path.append("/searchingthedashlegalnotice.html");

Why not just doing:
std::string legal_file_path = "file://" PKGDATADIR "/searchingthedashlegalnotice.html";

221 + nux::LayeredLayout* layered_layout_;
222 + nux::HLayout *legal_layout_;

It seems you don't need to keep these in class.

Finally, both the icon and the text are always built, only their visibility is switched, couldn't avoid this (and then only allocate only the needed resources)?

« Back to merge proposal