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

option to autodetect desktop wallpaper for use as backgroundImage #7295

Closed
cjwijtmans opened this issue Aug 14, 2020 · 13 comments · Fixed by #7849
Closed

option to autodetect desktop wallpaper for use as backgroundImage #7295

cjwijtmans opened this issue Aug 14, 2020 · 13 comments · Fixed by #7849
Labels
Area-Settings Issues related to settings and customizability, for console or terminal good first issue This is a fix that might be easier for someone to do as a first contribution Help Wanted We encourage anyone to jump in on these. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone
Terminal Backlog

Comments

@cjwijtmans
Copy link

cjwijtmans commented Aug 14, 2020 •
edited
Loading

My current setting for backgroundImage is

"backgroundImage": "C:\\Users\\<user>\\AppData\\Local\\Microsoft\\Windows\\Themes\\RoamedThemeFiles\\DesktopBackground\\<file>"

The problem is the file name is not static. So i would like an (default)option to autodetect dekstop wallpaper like

"backgroundImage": "DesktopBackground"
@cjwijtmans cjwijtmans added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label Aug 14, 2020
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Aug 14, 2020
@zadjii-msft
Copy link
Member

Is there an API to get the path of the current desktop background image? I'm not sure there is, but I've been wrong before.

I suppose this StackOverflow post has some answers. I'm not sure how stable those answers are, but they might work.

@zadjii-msft zadjii-msft added Area-Settings Issues related to settings and customizability, for console or terminal good first issue This is a fix that might be easier for someone to do as a first contribution Help Wanted We encourage anyone to jump in on these. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. and removed Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. labels Aug 14, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Aug 14, 2020
@zadjii-msft zadjii-msft added this to the Terminal Backlog milestone Aug 14, 2020
@DHowett DHowett removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Aug 18, 2020
@bennettnicholas
Copy link
Contributor

bennettnicholas commented Sep 20, 2020 •
edited
Loading

@zadjii-msft / @DHowett I am a student and looking to help out in my free time. This would be my first attempt at any open source issue and my first time helping on the Terminal project. Would this be something you guys would like my help with? This looks fairly straight forward.

@cjwijtmans
Copy link
Author

cjwijtmans commented Sep 21, 2020 •
edited
Loading

@bennettnicholas The problem is there is no standard way to get the background image on windows. So this might be more difficult than you believe.

Ok i found this documentation

https://docs.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-systemparametersinfoa

Take a look at SPI_GETDESKWALLPAPER.

@cjwijtmans
Copy link
Author

Then the second thing to consider is using file watcher API to update the backgroundimage when the desktop wallpaper is changed but i dont think its very neccesary.

@bennettnicholas
Copy link
Contributor

@cjwijtmans I think using the SPI would work the best.

@bennettnicholas
Copy link
Contributor

I have been attempting to implement this. I am new to the code base, so I am having some issues getting used to where things are and how to debug.

Correct me if I am wrong, but I thought checking for a string value such as "Desktop" where you normally place the path could be checked in void CascadiaSettings::_ValidateMediaResources() in the file CascadiaSettings.cpp alongside when we check if the path converts to a URI, but I am having some issue extracting the backgroundimagepath to a sting. If it is "Desktop" then we could replace the path with the SPI_GETDESKWALLPAPER path.

@cjwijtmans
Copy link
Author

another option would be to add another setting "backgroundImageWallpaper": true which will override the path used in the code where it pulls the "backgroundImage" setting.

@bennettnicholas
Copy link
Contributor

Yea I like that idea much more, it just will require me to know a bit more on how terminal processes the settings. I will look into it. Thanks.

@cjwijtmans
Copy link
Author

Did you look into profile settings?

@bennettnicholas
Copy link
Contributor

Not yet. I actually just started to process of interviewing with Microsoft, which consumed the last few days of mine. I will try looking more into it tonight.

@bennettnicholas
Copy link
Contributor

@cjwijtmans I am pretty close. I have it working, but it has some bugs. Trying to figure that out now.

image

@bennettnicholas
Copy link
Contributor

@cjwijtmans I fixed the bug. Seems to work as expected after some minor testing. Would you be willing to pull down my branch and test it yourself? https://github.com/bennettnicholas/terminal.git (Branch: autoDetectBackgroundImage)

This was referenced Oct 7, 2020
Merged
Merged
@ghost ghost added the In-PR This issue has a related PR label Oct 7, 2020
@ghost ghost closed this as completed in #7849 Oct 15, 2020
ghost pushed a commit that referenced this issue Oct 15, 2020
@bennettnicholas
Auto detect background image (#7849)
6e8388e 
##  Summary of the Pull Request
Added watch on desktopImagePath to check when the path equals "DesktopWallpaper"
If it does equal "DesktopWallpaper" it replaces the path with a path to the desktop's wallpaper

*I am a student and this is my first pull request for Terminal so please give feedback no matter how small. It's the best way I can learn.

## PR Checklist
* [X] Closes #7295 
* [X] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [?] Tests added/passed
* [X] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: MicrosoftDocs/terminal#155
* [?] Schema updated. (Not sure if this is needed, also not sure where this would be)
* [X] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #7295 (Have only talked with the people on the issue, which I don't think has any core contributors)

## Detailed Description of the Pull Request / Additional comments
I am using SystemParametersInfo for SPI_GETDESKWALLPAPER which puts the path into a WCHAR and that is then inserted as the BackgroundImagePath.

I do not think an additional test would add value. The SPI_GETDESKTOPWALLPAPER uses the computers local wallpaper path and puts it into a WCHAR, which then I feed into BackgroundImagePath() as it's new path. I don't think there adds value in making a static path of the desktop background and testing that, given that static tests are already done for "BackgroundImage()".

## Validation Steps Performed

(Manual Validation - Test False Value)
1. Ran Terminal
2. Set setting ["backgroundImage": "<some random img path>"] under profiles->defaults
3. Verified terminal's background is not the desktops wallpaper. 

(Manual Validation - Test True Value)
1. Ran Terminal
2. Set setting ["backgroundImage": "DesktopWallpaper"] under profiles->defaults
3. Verified the background image matches the desktop background image. 

(Manual Validation - Multiple Tabs True Value)
1. Ran Terminal
2. Set setting ["backgroundImage": "DesktopWallpaper"] under profiles->defaults
3. Verified the background image matches the desktop background image.  
4. Opened new tabs
5. Verified the background image matches the desktop background image for each tab.
@ghost ghost removed the In-PR This issue has a related PR label Oct 15, 2020
@ghost ghost added the Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. label Oct 15, 2020
@ghost
Copy link

ghost commented Nov 11, 2020

🎉This issue was addressed in #7849, which has now been successfully released as Windows Terminal Preview v1.5.3142.0.:tada:

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area-Settings Issues related to settings and customizability, for console or terminal good first issue This is a fix that might be easier for someone to do as a first contribution Help Wanted We encourage anyone to jump in on these. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Milestone
Terminal Backlog
Development

Successfully merging a pull request may close this issue.

Auto detect background image bennettnicholas/terminal
4 participants
@DHowett @cjwijtmans @zadjii-msft @bennettnicholas