Re: Review Request 117131: Implement KUser::faceIconPath on Windows.

[Alexander Richardson]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/117131/#review54437
---


I would personally keep the fallback code in case we couldn't load 
SHGetUserPicturePath (it is undocumented after all and could be removed in a 
future windows version), but if you want to remove it I'm also fine with that.


src/lib/util/kuser_win.cpp


Don't think we need the RAII object here, AFAIK we link to shell32.dll 
anyway, so it will only be deleted on process exit anyway.



src/lib/util/kuser_win.cpp


maybe make this static so it only gets resolved once:


static SGUPP_ptr SHGetUserPicturePath = 
reinterpret_cast(GetProcAddress(LoadLibraryW(L"shell32.dll"), 
MAKEINTRESOURCEA(261)));



src/lib/util/kuser_win.cpp


WCHAR pathBuf[MAX_PATH];

then you don't need the reinterpret_cast and can use 
QString::fromWCharArray()


- Alexander Richardson


On March 28, 2014, 2:07 a.m., Nicolás Alvarez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/117131/
> ---
> 
> (Updated March 28, 2014, 2:07 a.m.)
> 
> 
> Review request for KDE Frameworks and kdewin.
> 
> 
> Repository: kcoreaddons
> 
> 
> Description
> ---
> 
> Implement KUser::faceIconPath on Windows.
> 
> I use an undocumented Windows API 
> (http://undoc.airesoft.co.uk/shell32.dll/SHGetUserPicturePath.php) that 
> stores the profile image in a temporary file and returns the path to it. The 
> previous code was just trying to load that temporary file, and would only 
> work if another app had created the file recently (such as the control panel 
> section where the image is changed).
> 
> This only works on Windows Vista and later; on Windows XP the undocumented 
> API is different, and faceIconPath will just return an empty string.
> 
> 
> Diffs
> -
> 
>   src/lib/util/kuser_win.cpp 96cf2f0b89ac18a68783793d3a8b2827b72dd968 
> 
> Diff: https://git.reviewboard.kde.org/r/117131/diff/
> 
> 
> Testing
> ---
> 
> Compiled with MSVC2010, tested via the new faceicontest on Windows 7.
> 
> 
> Thanks,
> 
> Nicolás Alvarez
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 117131: Implement KUser::faceIconPath on Windows.

[Nicolás Alvarez]

> On March 28, 2014, 10:49 a.m., Alexander Richardson wrote:
> > src/lib/util/kuser_win.cpp, line 391
> > 
> >
> > WCHAR pathBuf[MAX_PATH];
> > 
> > then you don't need the reinterpret_cast and can use 
> > QString::fromWCharArray()

Ah nice, I didn't know about fromWCharArray, and without that, using WCHAR 
meant I needed a reinterpret_cast in the fromUtf16 call anyway. I'll change it.


> On March 28, 2014, 10:49 a.m., Alexander Richardson wrote:
> > src/lib/util/kuser_win.cpp, line 388
> > 
> >
> > maybe make this static so it only gets resolved once:
> > 
> > 
> > static SGUPP_ptr SHGetUserPicturePath = 
> > reinterpret_cast(GetProcAddress(LoadLibraryW(L"shell32.dll"), 
> > MAKEINTRESOURCEA(261)));

Agreed. And it makes sense to make the library handle static too.


> On March 28, 2014, 10:49 a.m., Alexander Richardson wrote:
> > src/lib/util/kuser_win.cpp, line 384
> > 
> >
> > Don't think we need the RAII object here, AFAIK we link to shell32.dll 
> > anyway, so it will only be deleted on process exit anyway.

-1. You could make the same argument about not needing to uninitialize COM. 
It's simply good practice to free what you allocate, or in this case, decrement 
reference counts when you're done even if you know it'll never reach 0 anyway.


- Nicolás


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/117131/#review54437
---


On March 27, 2014, 10:07 p.m., Nicolás Alvarez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/117131/
> ---
> 
> (Updated March 27, 2014, 10:07 p.m.)
> 
> 
> Review request for KDE Frameworks and kdewin.
> 
> 
> Repository: kcoreaddons
> 
> 
> Description
> ---
> 
> Implement KUser::faceIconPath on Windows.
> 
> I use an undocumented Windows API 
> (http://undoc.airesoft.co.uk/shell32.dll/SHGetUserPicturePath.php) that 
> stores the profile image in a temporary file and returns the path to it. The 
> previous code was just trying to load that temporary file, and would only 
> work if another app had created the file recently (such as the control panel 
> section where the image is changed).
> 
> This only works on Windows Vista and later; on Windows XP the undocumented 
> API is different, and faceIconPath will just return an empty string.
> 
> 
> Diffs
> -
> 
>   src/lib/util/kuser_win.cpp 96cf2f0b89ac18a68783793d3a8b2827b72dd968 
> 
> Diff: https://git.reviewboard.kde.org/r/117131/diff/
> 
> 
> Testing
> ---
> 
> Compiled with MSVC2010, tested via the new faceicontest on Windows 7.
> 
> 
> Thanks,
> 
> Nicolás Alvarez
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 117131: Implement KUser::faceIconPath on Windows.

[Nicolás Alvarez]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/117131/
---

(Updated March 30, 2014, 12:35 p.m.)


Review request for KDE Frameworks and kdewin.


Changes
---

Made library handle and function pointer static.
Changed type of buffer to WCHAR[].


Repository: kcoreaddons


Description
---

Implement KUser::faceIconPath on Windows.

I use an undocumented Windows API 
(http://undoc.airesoft.co.uk/shell32.dll/SHGetUserPicturePath.php) that stores 
the profile image in a temporary file and returns the path to it. The previous 
code was just trying to load that temporary file, and would only work if 
another app had created the file recently (such as the control panel section 
where the image is changed).

This only works on Windows Vista and later; on Windows XP the undocumented API 
is different, and faceIconPath will just return an empty string.


Diffs (updated)
-

  src/lib/util/kuser_win.cpp 96cf2f0 

Diff: https://git.reviewboard.kde.org/r/117131/diff/


Testing
---

Compiled with MSVC2010, tested via the new faceicontest on Windows 7.


Thanks,

Nicolás Alvarez

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 117131: Implement KUser::faceIconPath on Windows.

[Alexander Richardson]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/117131/#review54998
---

Ship it!


Ship It!


src/lib/util/kuser_win.cpp


You could keep the old code as a fallback here, but i don't really mind


- Alexander Richardson


On March 30, 2014, 5:35 p.m., Nicolás Alvarez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/117131/
> ---
> 
> (Updated March 30, 2014, 5:35 p.m.)
> 
> 
> Review request for KDE Frameworks and kdewin.
> 
> 
> Repository: kcoreaddons
> 
> 
> Description
> ---
> 
> Implement KUser::faceIconPath on Windows.
> 
> I use an undocumented Windows API 
> (http://undoc.airesoft.co.uk/shell32.dll/SHGetUserPicturePath.php) that 
> stores the profile image in a temporary file and returns the path to it. The 
> previous code was just trying to load that temporary file, and would only 
> work if another app had created the file recently (such as the control panel 
> section where the image is changed).
> 
> This only works on Windows Vista and later; on Windows XP the undocumented 
> API is different, and faceIconPath will just return an empty string.
> 
> 
> Diffs
> -
> 
>   src/lib/util/kuser_win.cpp 96cf2f0 
> 
> Diff: https://git.reviewboard.kde.org/r/117131/diff/
> 
> 
> Testing
> ---
> 
> Compiled with MSVC2010, tested via the new faceicontest on Windows 7.
> 
> 
> Thanks,
> 
> Nicolás Alvarez
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 117131: Implement KUser::faceIconPath on Windows.

[Commit Hook]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/117131/#review55008
---


This review has been submitted with commit 
c35ece00babf9a2ee0fd88a482e09ffa5cee9257 by Nicolás Alvarez to branch master.

- Commit Hook


On March 30, 2014, 3:35 p.m., Nicolás Alvarez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/117131/
> ---
> 
> (Updated March 30, 2014, 3:35 p.m.)
> 
> 
> Review request for KDE Frameworks and kdewin.
> 
> 
> Repository: kcoreaddons
> 
> 
> Description
> ---
> 
> Implement KUser::faceIconPath on Windows.
> 
> I use an undocumented Windows API 
> (http://undoc.airesoft.co.uk/shell32.dll/SHGetUserPicturePath.php) that 
> stores the profile image in a temporary file and returns the path to it. The 
> previous code was just trying to load that temporary file, and would only 
> work if another app had created the file recently (such as the control panel 
> section where the image is changed).
> 
> This only works on Windows Vista and later; on Windows XP the undocumented 
> API is different, and faceIconPath will just return an empty string.
> 
> 
> Diffs
> -
> 
>   src/lib/util/kuser_win.cpp 96cf2f0 
> 
> Diff: https://git.reviewboard.kde.org/r/117131/diff/
> 
> 
> Testing
> ---
> 
> Compiled with MSVC2010, tested via the new faceicontest on Windows 7.
> 
> 
> Thanks,
> 
> Nicolás Alvarez
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 117131: Implement KUser::faceIconPath on Windows.

[Nicolás Alvarez]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/117131/
---

(Updated April 5, 2014, 1:32 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and kdewin.


Repository: kcoreaddons


Description
---

Implement KUser::faceIconPath on Windows.

I use an undocumented Windows API 
(http://undoc.airesoft.co.uk/shell32.dll/SHGetUserPicturePath.php) that stores 
the profile image in a temporary file and returns the path to it. The previous 
code was just trying to load that temporary file, and would only work if 
another app had created the file recently (such as the control panel section 
where the image is changed).

This only works on Windows Vista and later; on Windows XP the undocumented API 
is different, and faceIconPath will just return an empty string.


Diffs
-

  src/lib/util/kuser_win.cpp 96cf2f0 

Diff: https://git.reviewboard.kde.org/r/117131/diff/


Testing
---

Compiled with MSVC2010, tested via the new faceicontest on Windows 7.


Thanks,

Nicolás Alvarez

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel