Tooltip notification. Check for update fixes.

src/plugin/NotificationMessage.cpp

 #include <Windows.h>
 #include <CommCtrl.h>
 
 #include "NotificationMessage.h"
 
-NotificationMessage::NotificationMessage()
-{
-  CommonControlsInitialize();
-}
-
-NotificationMessage::NotificationMessage(HWND parent)
+NotificationMessage::NotificationMessage(HWND parent): parentWindow(parent)
 { 
-  parentWindow = parent;
-  CommonControlsInitialize();
+  toolTipWindow = 0;
+  InitializeCommonControls();
 };
 
 bool NotificationMessage::commonControlsInitialized(false);
 
-void NotificationMessage::CommonControlsInitialize()
+void NotificationMessage::InitializeCommonControls()
 {
   if (!commonControlsInitialized)
   {
     INITCOMMONCONTROLSEX commControls;
     commControls.dwSize = sizeof(INITCOMMONCONTROLSEX);
     commControls.dwICC = ICC_BAR_CLASSES;
     InitCommonControlsEx(&commControls);
     commonControlsInitialized = true;
   }
 }
 
 bool NotificationMessage::Show(std::wstring message, std::wstring title, int icon)
 {
+  if (toolTipWindow != 0)
+  {
+    Hide();
+  }
   toolTipWindow = CreateWindowEx(WS_EX_TOPMOST, TOOLTIPS_CLASS, NULL,
                                   TTS_NOPREFIX | TTS_BALLOON | TTS_CLOSE,
                                   0, 0,
                                   0, 0,
                                   parentWindow, NULL, NULL,
                                   NULL);
 
   SetWindowPos(toolTipWindow, HWND_TOPMOST,0, 0, 0, 0,
           SWP_NOMOVE | SWP_NOSIZE | SWP_NOACTIVATE);
-  TOOLINFO ti;

[Wladimir Palant, 2013/09/19 08:59:16]

We should declare this as TOOLINFOW explicitly.

-        ti.cbSize = sizeof(TOOLINFO);

[Wladimir Palant, 2013/09/19 08:59:16]

Indentation is off, convert tabs to spaces?

+  TOOLINFOW ti = {};
+  ti.cbSize = sizeof(TOOLINFOW);
   ti.uFlags = TTF_IDISHWND | TTF_TRACK | TTF_TRANSPARENT;
-        ti.hwnd = toolTipWindow;
-        ti.hinst = NULL;
-        ti.uId = (UINT_PTR)parentWindow;
-        ti.lpszText = (LPWSTR)message.c_str();          

[Wladimir Palant, 2013/09/19 08:59:16]

I think that the conversion to LPWSTR is unnecessary here.

[Oleksandr, 2013/09/25 10:03:14]

We need to remove const here.

[Wladimir Palant, 2013/09/25 10:27:14]

Please use const_cast to indicate that. It's a pity that the Windows API is
badly designed with respect to const-ness, this is the best we can do I think.

+  ti.hwnd = toolTipWindow;
+  ti.hinst = NULL;
+  ti.uId = (UINT_PTR)parentWindow;
+  ti.lpszText = const_cast<LPWSTR>(message.c_str());
   GetClientRect(parentWindow, &ti.rect);
 
-        LRESULT res = ::SendMessage(toolTipWindow, TTM_ADDTOOL, 0, (LPARAM) (LPTOOLINFO) &ti);

[Wladimir Palant, 2013/09/19 08:59:16]

The conversion to LPTOOLINFO is unnecessary - &ti is already a pointer to
TOOLINFO.

+  LRESULT res = ::SendMessage(toolTipWindow, TTM_ADDTOOL, 0, (LPARAM)&ti);
 
   RECT rect;
   GetWindowRect(parentWindow, &rect);
   Move(rect.left + (rect.right - rect.left) / 2, rect.top + (rect.bottom - rect.top) / 2); 
 
-        res = ::SendMessage(toolTipWindow, TTM_SETTITLE, icon, (LPARAM)title.c_str());
-        res = ::SendMessage(toolTipWindow, TTM_TRACKACTIVATE, TRUE, (LPARAM)(LPTOOLINFO) &ti);

[Wladimir Palant, 2013/09/19 08:59:16]

Same here, the conversion to LPTOOLINFO is unnecessary. Also, assigning the
return value of SendMessage is unnecessary here and above if you don't actually
use the return value. Finally, that function shouldn't duplicate the
functionality from SetTextAndIcon() but rather call it.

+  SetTextAndIcon(message, title, icon);
+  res = ::SendMessage(toolTipWindow, TTM_TRACKACTIVATE, TRUE, (LPARAM)&ti);
 
   return true;
 }
 
-bool NotificationMessage::Hide()
+void NotificationMessage::Hide()
 {
-  DestroyWindow(toolTipWindow);
-  toolTipWindow = 0;
-  return true;
+  if (toolTipWindow != 0)
+  {
+    DestroyWindow(toolTipWindow);
+    toolTipWindow = 0;
+  }
 }
 
 void NotificationMessage::Move(short x, short y)
 {
-        ::SendMessage(toolTipWindow, TTM_TRACKPOSITION, 0, (LPARAM)(LPTOOLINFO)MAKELONG(x, y));

[Wladimir Palant, 2013/09/19 08:59:16]

The conversion to LPTOOLINFO is wrong here, that's not even a pointer we are
sending. In fact, I don't think any conversion is needed whatsoever.

+  ::SendMessage(toolTipWindow, TTM_TRACKPOSITION, 0, MAKELONG(x, y));
   return;
 }
 
 bool NotificationMessage::SetTextAndIcon(std::wstring text, std::wstring title, int icon)
 {
-  TOOLINFO ti;
-        ti.cbSize = sizeof(TOOLINFO);

[Wladimir Palant, 2013/09/19 08:59:16]

As above, this should be TOOLINFOW.

-  ti.uFlags = TTF_IDISHWND | TTF_TRACK | TTF_TRANSPARENT;
-        ti.hwnd = toolTipWindow;
-        ti.hinst = NULL;
-        ti.uId = (UINT_PTR)parentWindow;
-        ti.lpszText = (LPWSTR)text.c_str();             

[Wladimir Palant, 2013/09/19 08:59:16]

As above, the conversion to LPWSTR seems unnecessary.

-  GetClientRect(parentWindow, &ti.rect);
-        LRESULT res = ::SendMessage(toolTipWindow, TTM_SETTITLE, icon, (LPARAM)title.c_str());
-        res = ::SendMessage(toolTipWindow, TTM_UPDATETIPTEXT, 0, (LPARAM)&ti);
+  TOOLINFOW ti = {};
+  ti.cbSize = sizeof(TOOLINFOW);
+  ti.hwnd = toolTipWindow; 
+  ti.hinst = NULL;
+  ti.uId = (UINT_PTR)parentWindow;
+  ti.lpszText = const_cast<LPWSTR>(text.c_str());   
+  LRESULT res = ::SendMessage(toolTipWindow, TTM_SETTITLE, icon, (LPARAM)title.c_str());
+  res = ::SendMessage(toolTipWindow, TTM_UPDATETIPTEXT, 0, (LPARAM)&ti);
   return res == TRUE;
 }
 
 void NotificationMessage::SetParent(HWND parent)
 {
   parentWindow = parent;
 }
+
 bool NotificationMessage::IsVisible()

[Wladimir Palant, 2013/09/19 08:59:16]

Nit: Empty line before this function.

 {
   if (toolTipWindow == 0)
     return false;
   return IsWindowVisible(toolTipWindow);

[Wladimir Palant, 2013/09/19 08:59:16]

Given that we never hide the window but always destroy it - |return
toolTipWindow != 0;| should do in this function.

[Oleksandr, 2013/09/25 10:03:14]

The window may become hidden without us hiding it. (As a result of some user
interaction with top level windows, for example) 

[Wladimir Palant, 2013/09/25 10:27:14]

I see. We could react to WM_WINDOWPOSCHANGED then to destroy the window if it
was hidden. If we leave it as it is now we should make sure that we don't leak
that window - will NotificationMessage::Hide() be called nevertheless?

[Oleksandr, 2013/09/25 12:37:45]

Did that. Also if we do miss the NotificationMessage::Hide() for any reason, it
will be called next time the Window is meant to be created. I still think
actually querying the window handle here is better then just checking if it is
null.

On 2013/09/25 10:27:14, Wladimir Palant wrote:

 }