Code review comment for lp://staging/~ted/url-dispatcher/bad-url-prompt

On Wed, 2016-10-26 at 20:22 +0000, Charles Kerr wrote:
> > + * Copyright © 2015 Canonical Ltd.
> Wrong year. BLOCKER!!

You should check bzr blame before saying that, this branch has been
around a while ;-)

> > +# Builds the binary translations catalog for each language
> > +# it finds source translations (*.po) for
> > +foreach(POFILE ${POFILES})
> > +    string(REPLACE ".po" "" LANG ${POFILE})
> > +    list(APPEND PO_FILES "${POFILE}")
> > +    gettext_process_po_files(${LANG} ALL PO_FILES "${POFILE}")
> > +    set(INSTALL_DIR
> > ${CMAKE_INSTALL_LOCALEDIR}/${LANG}/LC_MESSAGES)
> > +    install(FILES ${CMAKE_CURRENT_BINARY_DIR}/${LANG}.gmo
> > +            DESTINATION ${INSTALL_DIR}
> > +            RENAME ${GETTEXT_PACKAGE}.mo
> > +    )
> > +endforeach(POFILE)
> This looks like it should work, so if there are compelling reasons to
> reinvent the wheel here that's fine.
>
> But before we land this, consider reusing existing code that does it,
> eg our cmake Intltool module. See indicator-network/po/CMakeLists.txt
> for an example

I don't remember the issue today, but I couldn't get that to work and
was told it is slightly broken. Happy to change, but want something
that works :-)

> > + g_debug("Setting up over lay for PID %d with
> > '%s'", (int)pid, data.appid.c_str());
> trivial: int(pid), here and below
>

Fixed r114

> > +void
> > +OverlayTrackerMir::removeSession (const std::string &type,
> > MirPromptSession * session)
> > +{
> > + g_debug("Removing session: %p", (void*)session);
> > +
> > + for (auto it = ongoingSessions[type].begin(); it !=
> > ongoingSessions[type].end(); it++) {
> > + if (it->session.get() == session) {
> > + ubuntu_app_launch_stop_multiple_helper(typ
> > e.c_str(), it->appid.c_str(), it->instanceid.c_str());
> > + ongoingSessions[type].erase(it);
> We lookup in the ongoingSessions map three times here, better to do
>
> auto& session = ongoingSessions[type];
> for (auto it = session.begin(); it != session.end(); ++it) {
>     ...
>     session.erase(it);

Good point. Fixed r115.