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.

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

Awake vNext - NOBLE_SIX_02162023 #24183

Merged
merged 36 commits into from
Mar 15, 2023
Merged

Awake vNext - NOBLE_SIX_02162023 #24183

merged 36 commits into from
Mar 15, 2023

Conversation

dend
Copy link
Collaborator

@dend dend commented Feb 17, 2023

Summary of the Pull Request

Implements quality-of-life improvements for Awake.

PR Checklist

  • Communication: I've discussed this with core contributors already. If work hasn't been agreed, this work might be rejected
  • Tests: Added/updated and all pass
  • Localization: All end user facing strings can be localized
  • Dev docs: Added/updated
  • Documentation updated: If checked, please file a pull request on our docs repo and link it here: #xxx

Copy link
Collaborator

@Jay-o-Way Jay-o-Way left a comment

Choose a reason for hiding this comment

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

Idea: since a time interval and a specific date/time both boil down to a specific point in time, do you think it's possible to (in the background) use one and the same setting?

@dend
Copy link
Collaborator Author

dend commented Feb 20, 2023

Idea: since a time interval and a specific date/time both boil down to a specific point in time, do you think it's possible to (in the background) use one and the same setting?

@JayOWay - I mean, kinda? Today what I am doing is using Reactive Extensions (Rx) to set up the proper timer that is either a point-in-time (date/time) or interval. There is an interesting scenario here that we need to be cognizant of that Rx should handle automatically - clock changes.

If I set the interval to 15 minutes, I will get my 15 minutes and that's it. If I set the clock to 12:30PM and then I go ahead and change the clock, I'd want Awake to be able to recognize the time and either end or extend the session.

@dend
Copy link
Collaborator Author

dend commented Mar 7, 2023

@jaimecbernardo @crutkas @stefansjfw could you please test and try reproducing the Awake settings page issue? Doesn't seem like I can repro it on my end.

@stefansjfw
Copy link
Collaborator

@jaimecbernardo @crutkas @stefansjfw could you please test and try reproducing the Awake settings page issue? Doesn't seem like I can repro it on my end.

I can reproduce this. Trying to navigate to Awake page crashes the settings app (both when running runner and the settings app directly)

@dend
Copy link
Collaborator Author

dend commented Mar 8, 2023

@stefansjfw is this after pulling the latest changes and doing a full (clean) rebuild?

@stefansjfw
Copy link
Collaborator

stefansjfw commented Mar 8, 2023

@stefansjfw is this after pulling the latest changes and doing a full (clean) rebuild?

Yes.

  • pulled the code
  • git clean -xfd
  • built the solution from VS

@dend
Copy link
Collaborator Author

dend commented Mar 8, 2023

@stefansjfw sorry, not Git clean - clean from VS, then full rebuild. I am going to be fiddling with the code later to see if I can repro the scenario. Can you share the logs for Awake in the meantime (see inside %LOCALAPPDATA%/Microsoft/PowerToys)?

@dend
Copy link
Collaborator Author

dend commented Mar 8, 2023

Here is what I see when I go to the Awake screen in the latest branch, so logs would be helpful to diagnose any of the issues you might have @Jay-o-Way @stefansjfw

Awake settings screen

@Jay-o-Way

This comment was marked as resolved.

@Jay-o-Way

This comment was marked as resolved.

@dend
Copy link
Collaborator Author

dend commented Mar 9, 2023

@Jay-o-Way let me run this with your settings and see what happens.

@dend
Copy link
Collaborator Author

dend commented Mar 9, 2023

This is in AwakeProperties.cs @Jay-o-Way:

image

I just ran the code with your settings and I do not get an error - the settings automatically get updated to the latest version that includes the expiration time.

Syntax is this:

{
    "properties": {
        "keepDisplayOn": true,
        "mode": 3,
        "intervalHours": 0,
        "intervalMinutes": 0,
        "expirationDateTime": "2122-01-01T00:00:00.9188088-08:00",
        "customTrayTimes": {}
    },
    "name": "Awake",
    "version": "1.0"
}

awake_expire_at is also the old settings syntax - I've cleaned up that a bit, as you can see above, so you might still have an old snapshot of the code on your machine. In AwakeProperties.cs - what do you see as the JsonPropertyName for all assigned properties? A screenshot should be enough.

Then again, I just ran the code with your settings and they should auto-update to the new format.

@dend
Copy link
Collaborator Author

dend commented Mar 9, 2023

@Jay-o-Way looking at your logs, seems like old builds and new builds are mixed in. I see this, for example:

[2023-03-03 21:16:05.6646 INFO Awake.Program] Launching Awake...
[2023-03-03 21:16:05.6646 INFO Awake.Program] 0.0.1.0
[2023-03-03 21:16:05.6646 INFO Awake.Program] Build: ARBITER_01312022

The latest build is NOBLE_SIX_02162023. I am guessing this is because you're running the PR as well as the stable Awake on your machine. While nothing obvious stands out from the logs - are you running them separately or are they running at the same time? Wonder if the installed version is conflicting somehow with the new one.

Fix the build log name
dend and others added 3 commits March 10, 2023 09:43
This specifically addresses the fact that the `ExpirationDateTime` property was incorrectly auto-initialized to `DateTime.MinValue` when it should've been set to `DateTimeOffset.MinValue` to be consistent with the underlying type and assumptions around date/time.
Update with date assignment fix
Update with latest date fix
@dend dend requested a review from stefansjfw March 10, 2023 17:46
Copy link
Collaborator

@stefansjfw stefansjfw left a comment

Choose a reason for hiding this comment

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

LGTM! I gave it a test, everything works well

@stefansjfw stefansjfw merged commit 4662527 into microsoft:main Mar 15, 2023
@dend dend deleted the update-resx branch March 15, 2023 20:48
x:Uid="Awake_ExpirationSettingsCard"
HeaderIcon="{ui:FontIcon FontFamily={StaticResource SymbolThemeFontFamily}, Glyph=}"
Visibility="{x:Bind ViewModel.IsExpirationConfigurationEnabled, Mode=OneWay}">
<StackPanel Orientation="Horizontal">
Copy link
Collaborator

Choose a reason for hiding this comment

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

@dend @niels9001 These controls stay horizontal and are not very responsive.
image

Copy link
Collaborator

@Jay-o-Way Jay-o-Way left a comment

Choose a reason for hiding this comment

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

Controls are not responsive for window width

@dend
Copy link
Collaborator Author

dend commented Mar 21, 2023

@Jay-o-Way will review and create another PR with the change.

BLM16 pushed a commit to BLM16/PowerToys that referenced this pull request Jun 22, 2023
* Initial scaffolding for expiration configuration

* Simplifying the code and adding support for expiration events

* Bit more cleanup

* Initial support for expirable keep-awake

* Update some of the threading logic

* Logging and timing consistency

* Initial UI scaffolding

* Fix pathing issue for the icon when using config file

* Add missing definitions

* Update with basic interface

* Cleanup redundant calls

* Update name per convention

* Simplify declaration

* Proper binding to secondary Time property

* Cleanup the terminology use

* Standardize naming conventions.

* More Awake cleanup

* Ability to update the UI when the tray icon updates

* Small tweaks before ViewModel refactor

* Refactor the view model logic

* Some consistency fixes

* Remove the build props change

* Add settings scaffolding when a file does not exist

* Update expect.txt

* Fix typos

* Update build in logs

* Updating based on discussion in microsoft#24183.
This specifically addresses the fact that the `ExpirationDateTime` property was incorrectly auto-initialized to `DateTime.MinValue` when it should've been set to `DateTimeOffset.MinValue` to be consistent with the underlying type and assumptions around date/time.

---------

Co-authored-by: Clint Rutkas <clint@rutkas.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs-Review This Pull Request awaits the review of a maintainer. Product-Awake Issues regarding the PowerToys Awake utility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants