-
Notifications
You must be signed in to change notification settings - Fork 17
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.
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
taskbar-clock-customization 1.0.6: #9
base: master
Are you sure you want to change the base?
Conversation
* Allows web content to optionally (but by default) rotate through web content blocks * Makes `WebContentsUpdateInterval` a float, allowing for fine-tuned control of update/rotation time * Set the default `WebContentsUpdateInterval` to `0.5` (30 seconds) instead of `10` minutes
$name: Web content update interval | ||
$description: The update interval, in minutes, of the web content | ||
- WebContentsIterate: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice idea. With this option, perhaps we'll want to add an optional WebContentsBlockEnd
option.
@@ -76,9 +76,12 @@ Only Windows 10 64-bit and Windows 11 are supported. | |||
- WebContentsMaxLength: 28 | |||
$name: Web content maximum length | |||
$description: Longer strings will be truncated with ellipsis | |||
- WebContentsUpdateInterval: 10 | |||
- WebContentsUpdateInterval: "0.5" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like that it becomes a string. It would be nice to have Windhawk support floating point numbers (probably by showing them as numbers and saving them as strings), but for now, it's not supported.
In this case, I don't think an update interval of less then 1 minute is very interesting, but a rotation interval is. So I'd add another option for rotation interval which will be a number of seconds.
void UpdateWebContent() | ||
{ | ||
DWORD dwLength; | ||
LPBYTE pUrlContent = GetUrlContent(g_settings.webContentsUrl, &dwLength); | ||
if (!pUrlContent) { | ||
return; | ||
} | ||
if (g_settings.webContentsIterate) { | ||
if (webContentsLast && pUrlContent == webContentsLast) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pUrlContent == webContentsLast
compares two pointers, not the content. In this case it will always be false. Also, the page might be different every time (e.g. if there's a timestamp or dynamic content) while the fetched items might not change, so perhaps it's better to extract the items and compare them.
@@ -686,7 +714,8 @@ void LoadSettings() | |||
g_settings.webContentsStart = Wh_GetStringSetting(L"WebContentsStart"); | |||
g_settings.webContentsEnd = Wh_GetStringSetting(L"WebContentsEnd"); | |||
g_settings.webContentsMaxLength = Wh_GetIntSetting(L"WebContentsMaxLength"); | |||
g_settings.webContentsUpdateInterval = Wh_GetIntSetting(L"WebContentsUpdateInterval"); | |||
g_settings.webContentsUpdateInterval = std::stof(Wh_GetStringSetting(L"WebContentsUpdateInterval")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have a memory leak here, the string returned by Wh_GetStringSetting
is never freed.
If you plan on addressing my comments, please note that a new version of the mod was released recently, and I changed the main branch name from |
WebContentsUpdateInterval
a float, allowing for fine-tuned control of update/rotation timeWebContentsUpdateInterval
to0.5
(30 seconds) instead of10
minutesNote: I have about 3 hours of prior experience in C++. While I have plenty of experience coding, and it runs fine on my Windows 11 PC, it certainly could have a fatal flaw.