Skip to content
This repository has been archived by the owner on Jan 8, 2024. It is now read-only.

use SHGetKnownFolderPath, local instead of roaming #126

Merged
merged 3 commits into from
Dec 9, 2016

Conversation

zanderz
Copy link
Contributor

@zanderz zanderz commented Dec 2, 2016

Use system APIs instead of relying on the environment variable. Matching PRs in client and kbfs.
@maxtaco @gabriel

if dir == "" {
return "", fmt.Errorf("No APPDATA env set")
dir, err := LocalDataDir()
if err != nil || dir == "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you want to break this up...

if err != nil { return "", err }
if dir == "" { return "", fmt.Errorf("No APPDATA env set") }


// F1B32785-6FBA-4FCF-9D55-7B8E7F157091
var (
FOLDERID_LocalAppData = GUID{0xF1B32785, 0x6FBA, 0x4FCF, [8]byte{0x9D, 0x55, 0x7B, 0x8E, 0x7F, 0x15, 0x70, 0x91}}
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe this varname could be FolderIDLocalAppData

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This name is directly from the API, it seems better that it matches: https://msdn.microsoft.com/en-us/library/windows/desktop/dd378457(v=vs.85).aspx

Copy link
Contributor

@gabriel gabriel Dec 5, 2016

Choose a reason for hiding this comment

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

Maybe we could put a comment here explaining what the var is and that it corresponds to FOLDERID_LocalAppData and the link to that page... Just want to keep the linter happy.

"golang.org/x/sys/windows/registry"
)

type GUID struct {
Copy link
Contributor

@gabriel gabriel Dec 5, 2016

Choose a reason for hiding this comment

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

I think a lot of these structs & methods could be private? ... e.g. type guid struct

Copy link
Contributor

@gabriel gabriel left a comment

Choose a reason for hiding this comment

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

Can you run gometalinter too thx


func coTaskMemFree(pv uintptr) {
syscall.Syscall(procCoTaskMemFree.Addr(), 1, uintptr(pv), 0, 0)
return
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't need this return?

folder := syscall.UTF16ToString((*[1 << 16]uint16)(unsafe.Pointer(pszPath))[:])

if len(folder) == 0 {
return "", errors.New("can't get AppData directory")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think most of our errors are title cased... "Can't...

var pszPath uintptr
r0, _, _ := procSHGetKnownFolderPath.Call(uintptr(unsafe.Pointer(&id)), uintptr(0), uintptr(0), uintptr(unsafe.Pointer(&pszPath)))
if r0 != 0 {
return "", errors.New("can't get FOLDERID_RoamingAppData")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think most of our errors are title cased... "Can't...

return "", errors.New("can't get FOLDERID_RoamingAppData")
}

defer coTaskMemFree(pszPath)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to check pszPath is invalid?

@gabriel
Copy link
Contributor

gabriel commented Dec 6, 2016

lg2m with some comments...

Bump the version too?

@zanderz zanderz merged commit d765a8a into master Dec 9, 2016
@zanderz zanderz deleted the zanderz/CORE-3904 branch December 9, 2016 03:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants