[EXPLORER] Managing hiding behavior for tray notification icons. [WIP] #418

Getequ · 2018-03-04T18:51:36Z

Here UI part only, selected behaviors not saved.
Also fixed saving changes for parameters "Show seconds" and "Group buttons"

sdk/include/psdk/windowsx.h

HBelusca · 2018-03-04T21:26:15Z

base/shell/explorer/notifyiconscust.cpp

+        ComboBox_SetCurSel(hCombo, lvItem.lParam);
+
+        ComboBox_SetItemData(hCombo, 0, iItem);
+        SetWindowPos(hCombo, HWND_TOP, 190 + lv.x, pt.y + lv.y, 135, 20, SWP_SHOWWINDOW);


What are these magic values "190" and "135"?

base/shell/explorer/notifyiconscust.cpp

HBelusca · 2018-03-04T21:29:59Z

base/shell/explorer/notifyiconscust.cpp

+            InitializeListView(hwnd);
+            break;
+        case WM_NOTIFY:
+            {


There is a level of indentation too much.

HBelusca · 2018-03-04T21:31:40Z

base/shell/explorer/notifyiconscust.cpp

+                    EndDialog(hwnd, IDCANCEL);
+                    break;
+                case IDC_NOTIFICATION_BEHAVIOUR:
+                    {


No need to add this extra level of indentation.

HBelusca · 2018-03-04T21:32:07Z

base/shell/explorer/notifyiconscust.cpp

+{
+    HWND hListView = GetDlgItem(hDialog, IDC_NOTIFICATION_LIST);
+    HWND hSysPager = FindWindowEx(hTrayNotify, NULL, L"SysPager", NULL);
+    HWND hToolbar  = FindWindowEx(hSysPager, NULL, L"ToolbarWindow32", NULL);


Isn't there a way to retrieve the correct sub-window directly from the classes? Cc @gigaherz .

this is a standard win32 window classes, not a custom classes with unique name. It's lucky that hTrayNotify has only one sysPager and pager has only one toolbar :)

I was asking this because these HWNDs are just members of the corresponding CSysPager... classes, so I was wondering whether you could somehow do a pSysPager->m_hWnd somehow (with the correct names to be determined...) and/or use a correct getter function for this...

Yeah, not a perfect way. Tried to get toolbar HWND from CSysPager with functions like CTrayNotifyWnd_GetToolbar(m_TrayNotifyInstance) -> CSysPagerWnd_GetToolbar(&m_pager) -> Toolbar.m_hWnd field give me not null handle but it has very weird behavior: TB_BUTTONCOUNT and Toolbar.GetButtonCount() return zero, but CNotifyToolbar.GetVisibleButtonCount() return nonzero value.

This is definitely an ugly way to implement an obscure feature...

@yagoulas plz check is it fine now? i added functions CTrayNotifyWnd_GetTrayToolbar / CSysPagerWnd_GetTrayToolbar for getting tray toolbar

base/shell/explorer/notifyiconscust.cpp

HBelusca

Some stuff to review :)

Getequ · 2018-03-05T03:47:51Z

@HBelusca all done!

ThFabba · 2018-03-05T11:02:08Z

base/shell/explorer/traywnd.cpp

@@ -2778,6 +2778,10 @@ class CTrayWindow :
            SetWindowPos(hWndInsertAfter, 0, 0, 0, 0, SWP_NOACTIVATE | SWP_NOMOVE | SWP_NOSIZE | SWP_SHOWWINDOW);
        }

+        g_TaskbarSettings.bHideInactiveIcons = newSettings->bHideInactiveIcons;
+        g_TaskbarSettings.bShowSeconds = newSettings->bShowSeconds;
+        g_TaskbarSettings.bGroupButtons = newSettings->bGroupButtons;


These last two lines don't seem related to tray icon hiding. What's their purpose?

just to save this flag too. yes, they not related to tray icons

bShowSeconds and bGroupButtons are saved by the modules that implement handling these two, No need to save them again.

oops, sorry ) rollback this

base/shell/explorer/notifyiconscust.cpp

HBelusca · 2018-03-05T11:11:53Z

base/shell/explorer/notifyiconscust.cpp

+    ZeroMemory(&lviBehavior, sizeof(lviBehavior));
+    lviBehavior.mask = LVIF_TEXT;
+    lviBehavior.iSubItem = 1;
+    lviBehavior.cchTextMax = 100;


Don't hardcode "100" but use _countof(szBehaviour).
I even think you could use here the LoadStringW functionality described here: " If [nBufferMax] parameter is 0, then lpBuffer receives a read-only pointer to the resource itself."
Or alternatively, use CStringW class since you are writing C++ code there.

CStringW please

i actually load it using LoadStringW with _countof . 100 was a copy-paste waste )

ThFabba · 2018-03-05T13:26:24Z

So... I'm a bit allergic to implementing UI with no functionality behind it. We already have lots of buttons and dialogs that do nothing, and they're all really confusing for users.

What can we do to avoid that? Actually implementing the feature would be one way. Somehow clearly indicating to the user that this is unimplemented could also work. Suggestions welcome.

Getequ · 2018-03-05T15:02:55Z

@ThFabba yes, there is a reason in your words. But i see that even not big PR can hang for months sinking in discussions, even if they have Approved mark.

Completely implemented function requires a lot of time, which people do not have after work. I contribute as much as I can

Here UI part only, selected behaviors not saved. Also fixed saving changes for parameters "Show seconds" and "Group buttons"

HBelusca · 2018-03-12T14:26:03Z

You can either add the new UI elements ni the resources but hide them (using a | NOT WS_VISIBLE for each of them in the resources), and/or add button handling to pop up a "not yet implemented" message box.

Getequ · 2018-03-13T12:56:47Z

No, this is also not an option) i better finish it with as possible functionality as i can. I recently found this issue https://jira.reactos.org/browse/CORE-10849 - this was VERY helpful

All comments addressed

learn-more · 2018-06-12T20:39:44Z

So where are we going with this?

Getequ · 2018-06-13T05:05:26Z

I stuck with saving binary registy values. And also revealed new bug - importing files with big HEX registry values wrongly counting and read lines (regproc.c / processRegLinesW).

gonzoMD · 2018-07-09T15:52:18Z

Maybe @katahiromz can help you with that. He made some great explorer/shell fixes recently.

base/shell/explorer/resource.h

HBelusca · 2018-07-09T16:00:01Z

base/shell/explorer/notifyiconscust.cpp

+        WCHAR sInactive[100];
+        LoadStringW(NULL, IDS_NOTIF_BEH_SHOW, sShow, _countof(sShow));
+        LoadStringW(NULL, IDS_NOTIF_BEH_HIDE, sHide, _countof(sHide));
+        LoadStringW(NULL, IDS_NOTIF_BEH_HIDE_INACTIVE, sInactive, _countof(sInactive));


I'm hesitating in using CStringW class for the resource strings here (they would automatically deal correctly with the string sizes). @learn-more , any advice on that?

Why would you hestitate?
If you (for some obscure reason) don't want to use CStrings, you can always use the LoadStringW technique of passing it a pointer, and itll point that to the raw resource string (but be careful, it is not always null terminated!)

Usage:

CStringW str...(MAKEINTRESOURCEW(IDS_...));

HBelusca · 2018-07-09T16:00:41Z

base/shell/explorer/notifyiconscust.cpp

+
+    WCHAR szBehavior[100];
+    int resId = iState == 0 ? IDS_NOTIF_BEH_SHOW : (iState == 1 ? IDS_NOTIF_BEH_HIDE : IDS_NOTIF_BEH_HIDE_INACTIVE);
+    LoadStringW(NULL, resId, szBehavior, _countof(szBehavior));


Perhaps some CStringW usage? (see comment above)

HBelusca · 2018-07-09T16:00:49Z

base/shell/explorer/notifyiconscust.cpp

+    HWND hListView = GetDlgItem(hwnd, IDC_NOTIFICATION_LIST);
+    int count = ListView_GetItemCount(hListView);
+    WCHAR szBehavior[100];
+    LoadStringW(NULL, IDS_NOTIF_BEH_HIDE_INACTIVE, szBehavior, _countof(szBehavior));


HBelusca · 2018-07-09T16:01:57Z

base/shell/explorer/notifyiconscust.cpp

+            {
+                ShowBehaviorCombo(hwnd, (LPNMITEMACTIVATE)lParam);
+            }
+            else return FALSE;


Because first if-block was inside brackets, the return-FALSE should be inside brackets too (cf. our coding style).

HBelusca · 2018-07-09T16:02:59Z

base/shell/explorer/notifyiconscust.cpp

+        case WM_INITDIALOG: 
+            InitializeListView(hwnd);
+            break;
+        case WM_NOTIFY:


Please add one new-line between the different 'cases' to make the code more easily readable.

HBelusca · 2018-07-09T16:03:25Z

base/shell/explorer/notifyiconscust.cpp

+
+    LoadStringW(NULL, IDS_NOTIF_BEH_SHOW, sShow, _countof(sShow));
+    LoadStringW(NULL, IDS_NOTIF_BEH_HIDE, sHide, _countof(sHide));
+    LoadStringW(NULL, IDS_NOTIF_BEH_HIDE_INACTIVE, sInactive, _countof(sInactive));


HBelusca · 2018-07-09T16:04:35Z

base/shell/explorer/syspager.cpp

-    // Must keep a separate copy since the original is unioned with uTimeout.
-    UINT uVersionCopy;
-};
-


Why isn't this needed anymore?

No, it is moved to shell/explorer/precomp.h

ah yes sorry, I didn't see it.

Getequ · 2018-07-10T07:48:52Z

@gonzoMD hi, for now i waiting for accepting patch in Wine for reading imageList ( fix in #619 ). Without them this window will not look fine on ROS, only on Windows - i tested it by copying compiled explorer to XP in VBox

katahiromz · 2018-07-10T10:34:39Z

For more details, see "sdk/lib/atl/cstringt.h".

Getequ commented Mar 4, 2018

View reviewed changes

sdk/include/psdk/windowsx.h Outdated Show resolved Hide resolved

HBelusca reviewed Mar 4, 2018

View reviewed changes

base/shell/explorer/notifyiconscust.cpp Outdated Show resolved Hide resolved

HBelusca reviewed Mar 4, 2018

View reviewed changes

base/shell/explorer/notifyiconscust.cpp Outdated Show resolved Hide resolved

HBelusca reviewed Mar 4, 2018

View reviewed changes

base/shell/explorer/notifyiconscust.cpp Outdated Show resolved Hide resolved

HBelusca previously requested changes Mar 4, 2018

View reviewed changes

Getequ force-pushed the trayicons branch from f8bcf37 to 9d3994a Compare March 5, 2018 03:46

ThFabba reviewed Mar 5, 2018

View reviewed changes

HBelusca reviewed Mar 5, 2018

View reviewed changes

base/shell/explorer/notifyiconscust.cpp Outdated Show resolved Hide resolved

HBelusca reviewed Mar 5, 2018

View reviewed changes

Getequ force-pushed the trayicons branch from fd23011 to 76c40d0 Compare March 5, 2018 13:43

Getequ force-pushed the trayicons branch from 2a004d7 to 5f7aa85 Compare March 5, 2018 15:22

[EXPLORER] Managing hiding behavior for tray notification icons.

8799ebf

Here UI part only, selected behaviors not saved. Also fixed saving changes for parameters "Show seconds" and "Group buttons"

Getequ force-pushed the trayicons branch from c5dd667 to 8799ebf Compare March 8, 2018 14:04

Getequ changed the title ~~[EXPLORER] Managing hiding behavior for tray notification icons.~~ [EXPLORER] Managing hiding behavior for tray notification icons. [WIP] Mar 8, 2018

sanchaez added the WIP label Mar 12, 2018

sanchaez added the enhancement For PRs with an enhancement/new feature. label Apr 7, 2018

HBelusca reviewed Jul 9, 2018

View reviewed changes

tkreuzer added this to Changes requested or WIP in ReactOS PRs Dec 9, 2018

.

c98d65b

Getequ force-pushed the trayicons branch from a60ec42 to c98d65b Compare January 21, 2019 01:26

binarymaster marked this pull request as draft August 18, 2020 23:04

binarymaster removed the WIP label Aug 18, 2020

reactos deleted a comment from Saibamen Aug 18, 2020

Getequ closed this Jan 8, 2021

ReactOS PRs automation moved this from WIP / Waiting on contributor to Done Jan 8, 2021

binarymaster added the incomplete PRs that were closed without a real reason (check out later) label Jan 8, 2021

[EXPLORER] Managing hiding behavior for tray notification icons. [WIP] #418

[EXPLORER] Managing hiding behavior for tray notification icons. [WIP] #418

Conversation

Getequ commented Mar 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HBelusca left a comment • edited

Choose a reason for hiding this comment

Getequ commented Mar 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThFabba commented Mar 5, 2018

Getequ commented Mar 5, 2018 • edited

HBelusca commented Mar 12, 2018

Getequ commented Mar 13, 2018

learn-more commented Jun 12, 2018

Getequ commented Jun 13, 2018

gonzoMD commented Jul 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Getequ commented Jul 10, 2018

katahiromz commented Jul 10, 2018

HBelusca left a comment •

edited

Getequ commented Mar 5, 2018 •

edited