Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[SHELL32] Fix desktop.ini folder icon #595

Merged
merged 7 commits into from Jun 22, 2018
Merged

[SHELL32] Fix desktop.ini folder icon #595

merged 7 commits into from Jun 22, 2018

Conversation

katahiromz
Copy link
Contributor

@katahiromz katahiromz commented Jun 4, 2018 •
edited

Purpose

The folder icon location specified in desktop.ini of a folder should accept its relative path.
JIRA issue: CORE-9196

@katahiromz
Copy link
Contributor Author

After:
folder-icon-done

@sanchaez sanchaez added the bugfix For bugfix PRs. label Jun 4, 2018
HBelusca
HBelusca requested changes Jun 21, 2018
View reviewed changes
dll/win32/shell32/folders/CFSFolder.cpp Outdated
break;

// read-only or system folder?
const DWORD dwFileAttrs = _ILGetFileAttributes(ILFindLastID(pidl), NULL, 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"const" is not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also doesnt hurt?

dll/win32/shell32/folders/CFSFolder.cpp Outdated
{
ExpandEnvironmentStringsW(wszPath, szIconFile, cchMax);
// wszTemp --> szIconFile
GetFullPathNameW(wszTemp, cchMax, szIconFile, NULL);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to avoid the usage of the thread-unsafe SetCurrentDirectory function?
I was thinking about using some FindFirstFile... but maybe other solutions exist?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PathCombine or something in shlwapi shall be used. I will try to implement it tomorrow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used PathIsRelativeW and PathAppendW. Done.

dll/win32/shell32/folders/CFSFolder.cpp Outdated
}

static const WCHAR folder[] = { 'F', 'o', 'l', 'd', 'e', 'r', 0 };
} while (0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please avoid that construct in main code. This is part of the few exceptions where the "break;" you have above can be replaced by a "goto Quit;", and a "Quit:" label be used before the "getDefaultIconLocation2 call.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK.

HBelusca
HBelusca approved these changes Jun 21, 2018
View reviewed changes
dll/win32/shell32/folders/CFSFolder.cpp Outdated
{
WCHAR wszPath[MAX_PATH];
WCHAR wszCLSIDValue[CHARS_IN_GUID];
// build a full path of ini file
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"build the full path to the ini file"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK.

@HBelusca
Copy link
Contributor 

../../dll/win32/shell32/folders/CFSFolder.cpp: In function 'HRESULT getIconLocationForFolder(IShellFolder*, LPCITEMIDLIST, UINT, LPWSTR, UINT, int*, UINT*)':
../../dll/win32/shell32/folders/CFSFolder.cpp:239:1: error: jump to label 'Quit' [-fpermissive]
../../dll/win32/shell32/folders/CFSFolder.cpp:175:14: error:   from here [-fpermissive]
../../dll/win32/shell32/folders/CFSFolder.cpp:178:11: error:   crosses initialization of 'DWORD dwFileAttrs'

To fix this error, you must move the DWORD dwFileAttrs; declaration to the beginning of the function.

@HBelusca HBelusca merged commit f8926dc into reactos:master Jun 22, 2018
@katahiromz katahiromz deleted the folder-icon branch June 22, 2018 13:58
@yagoulas
Copy link
Member

Can you test if custom folder icons still work in places other than the desktop?

@katahiromz
Copy link
Contributor Author

You have to modify the file attributes and refresh the view.

@katahiromz
Copy link
Contributor Author

FAILED.
icon-failed
Something is wrong...

@yagoulas
Copy link
Member

I left a comment here: f8926dc#r29471950

@katahiromz katahiromz mentioned this pull request Jun 23, 2018
Merged
HBelusca pushed a commit that referenced this pull request Jun 24, 2018
@katahiromz @HBelusca
[SHELL32] Fix desktop.ini folder icon again (retrial of #595) (#632)
1829f5f 
#595 failed at non-Desktop folder. We should use ILGetDisplayNameExW instead of SHGetPathFromLDList to get path from psf and pidl.
CORE-9196
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@learn-more learn-more learn-more left review comments

@HBelusca HBelusca HBelusca approved these changes

Assignees
No one assigned
Labels
bugfix For bugfix PRs.
Projects
None yet
Milestone
No milestone
5 participants
@katahiromz @HBelusca @yagoulas @learn-more @sanchaez