[SHELL32][BROWSEUI] Implement ShellBrowser Apply/Reset default folder settings #6912

whindsaks · 2024-05-20T00:05:37Z

Notes:

~~I don't know the format of the Windows "Settings" stream. I made our version incompatible on purpose, it is much smaller and uses a special signature.~~
DefView only understands the Reset part, I'm not sure how/what to store as its settings yet.
SHELL_ErrorBox is a template to make it harder to pass in a NULL HWND on purpose.

HBelusca · 2024-05-20T10:08:15Z

I don't know the format of the Windows "Settings" stream. I made our version incompatible on purpose, it is much smaller and uses a special signature.

Well that's precisely what we try to avoid doing in ReactOS (making the exported interfaces incompatible on purpose).
Also, it's 99% possible the stream is of the usual IStream format: https://git.reactos.org/?p=reactos.git&a=search&h=HEAD&st=grep&s=IStream

Maybe one of the other shell guys could help you determine what the actual format is.

sdk/include/reactos/shlobj_undoc.h

dll/win32/shell32/precomp.h

dll/win32/shell32/dialogs/view.cpp

HBelusca · 2024-05-20T10:17:43Z

dll/win32/browseui/globalfoldersettings.cpp

+                   FWF_NOSUBFOLDERS | FWF_NOSCROLL | FWF_NOENUMREFRESH | FWF_NOVISIBLE);
+}
+
+static void EnsureValid(DEFFOLDERSETTINGS &dfs)


Is this EnsureValid helper meant to be used in more than one place in the future? (So far it's only used in CGlobalFolderSettings::Load )

Suggested change

static void EnsureValid(DEFFOLDERSETTINGS &dfs)

static void EnsureValid(DEFFOLDERSETTINGS& dfs)

No sure yet. Not in this PR anyway. External components should probably use IGlobalFolderSettings if they want to read but so far I think only CShellBrowser needs this data and I made its SBFOLDERSETTINGS look like the other structs it is currently reading with a .Load method.

dll/win32/browseui/globalfoldersettings.cpp

katahiromz · 2024-05-20T10:31:28Z

dll/win32/browseui/globalfoldersettings.cpp

+                   FWF_NOSUBFOLDERS | FWF_NOSCROLL | FWF_NOENUMREFRESH | FWF_NOVISIBLE);
+}
+
+static void EnsureValid(DEFFOLDERSETTINGS &dfs)


Suggested change

static void EnsureValid(DEFFOLDERSETTINGS &dfs)

static void EnsureValid(DEFFOLDERSETTINGS& dfs)

dll/win32/browseui/shellbrowser.cpp

katahiromz · 2024-05-20T10:35:40Z

dll/win32/shell32/CDefView.cpp

+                FIXME("Save current as default\n");
+                break;
+            case DVCMDID_RESETDEFAULTFOLDERSETTINGS:
+                wsprintfW(SubKey, L"%s\\%s", REGSTR_PATH_EXPLORER, L"Streams\\Default");


Suggested change

wsprintfW(SubKey, L"%s\\%s", REGSTR_PATH_EXPLORER, L"Streams\\Default");

StringCchPrintfW(SubKey, _countof(SubKey), L"%s\\%s", REGSTR_PATH_EXPLORER, L"Streams\\Default");

Why? The string will always fit.

whindsaks · 2024-05-20T10:47:57Z

Well that's precisely what we try to avoid doing in ReactOS (making the exported interfaces incompatible on purpose).

I see two options:

A) Try to make it Windows compatible. The problem with this is that the registry value is 40 bytes and I'm not going to be able to figure out all the bytes. Making it 40 bytes and let Windows load garbage/unknown data is not a great option. This value is also 4 bytes smaller on 2000 than on XP and there is no size of it stored inside like a classic "cbSize". Every time I Apply/Reset, a member is incremented as some sort of counter, I assume so Windows knows if it has loaded the up to date version or a stale one.

B) Be incompatible on purpose. This is not a problem. The only scenario where something goes "wrong" is when you first use ROS browseui and then Windows browseui on the same system (or vice versa). In this case it will fail to read the users default preference and you will end up with the "system defaults". When Windows reads our "corrupt" value, I believe it ends up in either VIEW_PRIORITY_STALECACHEHIT or VIEW_PRIORITY_CACHEMISS mode.

Just being able to load/save this value is a big win for ROS, it means you can set a different view mode as the default for all FS folders if you don't like ListView report mode.

Also, it's 99% possible the stream is of the usual IStream format

IStream is not a format! That is like saying "file X is probably of CreateFile format". It is not an OLE stream with the class CLSID stored first.

Save/Restore the state of the ShellBrowser toolbar/addressbar/statusbar. This is related to CORE-9094. Windows shares the state of the Go button and the locked state between Explorer and Internet Explorer but the bar states are not shared. Notes: - This is not really related to PR reactos#6912. This PR deals with the ShellBrowser state that is saved every time you change the rebar while the other is only about the default folder settings.

JoachimHenze · 2024-05-23T18:40:12Z

I don't know the format of the Windows "Settings" stream. I made our version incompatible on purpose, it is much smaller and uses a special signature.

Well that's precisely what we try to avoid doing in ReactOS (making the exported interfaces incompatible on purpose). Also, it's 99% possible the stream is of the usual IStream format: https://git.reactos.org/?p=reactos.git&a=search&h=HEAD&st=grep&s=IStream

Maybe one of the other shell guys could help you determine what the actual format is.

I do second @HBelusca . We do want to be compatible.
There are 3rd-party-tools for tweaking that stuff, and if we implemented incompatible, ros would be incompatible to those tools. Not good.

Many ShellBags-feature-reversing/documentation can be found in the description of the following ticket. https://jira.reactos.org/browse/CORE-9283
and the following link will turn out to be exceptionally useful in your context:
https://superuser.com/questions/253249/windows-registry-entries-for-default-explorer-view/253267

tkreuzer · 2024-05-24T04:52:33Z

I don't know the format of the Windows "Settings" stream. I made our version incompatible on purpose, it is much smaller and uses a special signature.

Well that's precisely what we try to avoid doing in ReactOS (making the exported interfaces incompatible on purpose). Also, it's 99% possible the stream is of the usual IStream format: https://git.reactos.org/?p=reactos.git&a=search&h=HEAD&st=grep&s=IStream

What we should also try to avoid, is painstakingly trying to be "binary compatible" with some internal interface that some version of Windows used to have 30 years ago and that not a single application outside of internal MS tools ever used.
That would prevent code like this in our code base.

    if ((param->dwFlags & 0x80000) == 0 && param->offset80 != NULL)
        ILFree(param->offset80);

learn-more · 2024-05-24T12:18:30Z

As I said on mattermost:
If the choice is made to make this incompatible with MS's interface (which should be fine I guess), reflect this in the name so that it is clearly not compatible. (f.e. "ReactOS-Streams", or drop it under the ReactOS subkey instead of Microsoft)

whindsaks · 2024-05-24T13:34:32Z

I can use a different name for the value but it should still be in the same subkey so that anyone nuking the key used by Windows also nukes us.

HBelusca · 2024-05-30T15:55:31Z

dll/win32/browseui/globalfoldersettings.cpp

+
+static void InitializeDefaults(DEFFOLDERSETTINGS &dfs)
+{
+    C_ASSERT(FIELD_OFFSET(DEFFOLDERSETTINGS, FolderSettings) == 4);


is this 4 corresponding to a sizeof(DWORD)?

I don't know what the Windows struct has as the first member, a DWORD or bit-field or two WORDs etc. I just know the ListView view mode is 4 bytes into the struct.

dll/win32/browseui/shellbrowser.cpp

sdk/include/reactos/shlobj_undoc.h

dll/win32/browseui/globalfoldersettings.cpp

HBelusca · 2024-05-30T16:02:10Z

sdk/include/reactos/shellutils.h

+SHELL_ErrorBoxHelper(HWND hwndOwner, UINT Error)
+{
+    WCHAR buf[400];
+    UINT cch, u32_errstr = 2;


2 ? What's this error corresponding to?

It is the resource id for the string that says "Error" (and only used when there is no other option if FormatMessage fails). This id is the same on Windows.

You could do UINT cch, u32_errstr = IDS_ERROR;
Then at the top of the file you can add:

#ifndef IDS_ERROR #define IDS_ERROR 2 #endif // IDS_ERROR

I'm guessing we call it IDS_ERROR in our resources, but double check that.

This is shellutils.h, I don't want to pollute globally with macros that may cause conflicts. I will however use a more descriptive name that includes "IDS_ERROR".

It will not cause an issue. It's encouraged in fact.
From Microsoft's MSVC documentation:

"However, you can redefine the macro with exactly the same definition. Thus, the same definition may appear more than once in a program."

From GNU's GCC documentation:

"If the new definition is effectively the same, the redefinition is silently ignored. This allows, for instance, two different headers to define a common macro. The preprocessor will only complain if the definitions do not match."

https://learn.microsoft.com/en-us/cpp/preprocessor/macros-c-cpp?view=msvc-170
https://gcc.gnu.org/onlinedocs/cpp/Undefining-and-Redefining-Macros.html

Maybe MSI shouldn't redefine IDS_ERROR? MSI is winesynced, it would never use shellutils.h. And if you did end up forking it to use shellutils.h, you should give that resource a different name; it's poorly named.

Those are fair points.

I still don't understand how a define is better than something contained inside the function in question.

No matter how we look at it, I'm relying on a user32 implementation detail and that is slightly hacky. But again, it's the best I can do...

The define points to why we set it to that value; it corresponds with IDS_ERROR resource in user32. You can add a comment to the top of the define block that says // From user32 resource IDS_ERROR or something similar. It may be similarly hacky, but it is more apparent that it is deliberate this way.

How about an enum (inside function) with a comment?

I guess that's better, I would dismiss my rejection if you did that. But I still think the define is a better way to do this.

HBelusca · 2024-05-30T16:50:29Z

sdk/include/reactos/shellutils.h

+    if (!cch)
+    {
+        cch = LoadStringW(LoadLibraryW(L"USER32"), u32_errstr, buf, _countof(buf));
+        wsprintfW(buf + cch, L"\n\n%#x (%d)", Error, Error);


Looks like a candidate for StringCchPrintfW here, though.

This is shellutils.h, I don't want to pull in anything. Besides, the string "Error" in any language + two 32-bit formatted numbers is going to fit in WCHAR buf[400];.

Why not? It's not just about fitting into your buffer. Using unsafe buffer functions can lead to security issues.

It's not just about fitting into your buffer. Using unsafe buffer functions can lead to security issues.

It is only about fitting it into the buffer. We know the format string is safe because we control it here.

cbialorucki · 2024-05-31T03:20:16Z

sdk/include/reactos/shellutils.h

@@ -98,7 +98,7 @@ static inline UINT
 SHELL_ErrorBoxHelper(HWND hwndOwner, UINT Error)
 {
    WCHAR buf[400];
-    UINT cch, u32_errstr = 2;
+    UINT cch, user32_IDS_ERROR = 2;


Still a magic constant. Please also see my other comments.

How would it be less magic if it was a define?

You can use a newly-added parameter.

You can use a newly-added parameter

I have no idea what this even means in this context

If this needs to be reused elsewhere, adding a #define USER32_ERROR 2 in our sdk/include/reactos/undocuser.h would make sense (+ a comment about what this define is for).

If this needs to be reused elsewhere

That is a big if.

Kernel32 and advapi32 should not be displaying a lot of message boxes. Other components higher up?

Cross that bridge when we get there?

Agreed 👍

sdk/include/reactos/shellutils.h

dll/win32/shell32/CFolderOptions.cpp

dll/win32/shell32/dialogs/view.cpp

HBelusca · 2024-05-31T13:20:11Z

sdk/include/reactos/shellutils.h

@@ -98,7 +98,7 @@ static inline UINT
 SHELL_ErrorBoxHelper(HWND hwndOwner, UINT Error)
 {
    WCHAR buf[400];
-    UINT cch, u32_errstr = 2;
+    UINT cch, user32_IDS_ERROR = 2;


If this needs to be reused elsewhere, adding a #define USER32_ERROR 2 in our sdk/include/reactos/undocuser.h would make sense (+ a comment about what this define is for).

dll/win32/browseui/settings.h

[SHELL32][BROWSEUI] Implement Apply/Reset default folder settings

3f3ad84

whindsaks requested review from learn-more and yagoulas as code owners May 20, 2024 00:05

github-actions bot added the shell All PR's related to the shell (and shell extensions) label May 20, 2024

whindsaks added the enhancement For PRs with an enhancement/new feature. label May 20, 2024

Moved ApplyBrowserDefaultFolderSettings declaration

c03461d

HBelusca requested review from HBelusca, katahiromz and cbialorucki May 20, 2024 10:08

HBelusca reviewed May 20, 2024

View reviewed changes

sdk/include/reactos/shlobj_undoc.h Outdated Show resolved Hide resolved

HBelusca reviewed May 20, 2024

View reviewed changes

katahiromz reviewed May 20, 2024

View reviewed changes

Use IGlobalFolderSettings to reset

8b300ee

DarkFire01 approved these changes May 23, 2024

View reviewed changes

Use struct similar to Windows

396e5da

whindsaks requested a review from HBelusca May 30, 2024 15:55

HBelusca reviewed May 30, 2024

View reviewed changes

whindsaks added 2 commits May 30, 2024 19:55

Some suggested changes

6a0543b

IDS_ERROR

7a45dcd

cbialorucki requested changes May 31, 2024

View reviewed changes

katahiromz reviewed May 31, 2024

View reviewed changes

sdk/include/reactos/shellutils.h Show resolved Hide resolved

My final offer

a28154f

HBelusca reviewed May 31, 2024

View reviewed changes

dll/win32/browseui/settings.h Show resolved Hide resolved

Suggested style change

b83b033

HBelusca approved these changes Jun 1, 2024

View reviewed changes

whindsaks merged commit c97c1ad into reactos:master Jun 4, 2024
34 checks passed

whindsaks deleted the ShFolderoptsSetDefSettings branch June 4, 2024 16:37

	static void EnsureValid(DEFFOLDERSETTINGS &dfs)
	static void EnsureValid(DEFFOLDERSETTINGS& dfs)

	wsprintfW(SubKey, L"%s\\%s", REGSTR_PATH_EXPLORER, L"Streams\\Default");
	StringCchPrintfW(SubKey, _countof(SubKey), L"%s\\%s", REGSTR_PATH_EXPLORER, L"Streams\\Default");

[SHELL32][BROWSEUI] Implement ShellBrowser Apply/Reset default folder settings #6912

[SHELL32][BROWSEUI] Implement ShellBrowser Apply/Reset default folder settings #6912

Conversation

whindsaks commented May 20, 2024 • edited

HBelusca commented May 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whindsaks May 20, 2024 • edited

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

whindsaks commented May 20, 2024 • edited

JoachimHenze commented May 23, 2024 • edited

tkreuzer commented May 24, 2024

learn-more commented May 24, 2024 • edited

whindsaks commented May 24, 2024

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

cbialorucki May 30, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbialorucki May 31, 2024 • edited

Choose a reason for hiding this comment

cbialorucki May 31, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbialorucki May 31, 2024 • edited

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

cbialorucki May 31, 2024 • edited

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 May 31, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HBelusca May 31, 2024 • edited

Choose a reason for hiding this comment

whindsaks commented May 20, 2024 •

edited

HBelusca commented May 20, 2024 •

edited

whindsaks May 20, 2024 •

edited

whindsaks commented May 20, 2024 •

edited

JoachimHenze commented May 23, 2024 •

edited

learn-more commented May 24, 2024 •

edited

cbialorucki May 30, 2024 •

edited

cbialorucki May 31, 2024 •

edited

cbialorucki May 31, 2024 •

edited

cbialorucki May 31, 2024 •

edited

cbialorucki May 31, 2024 •

edited

HBelusca May 31, 2024 •

edited

HBelusca May 31, 2024 •

edited