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

Launch/Open/GoTo: Code #32516

Closed
wants to merge 7 commits into from
Closed

Launch/Open/GoTo: Code #32516

wants to merge 7 commits into from

Conversation

Jay-o-Way
Copy link
Collaborator

@Jay-o-Way Jay-o-Way commented Apr 18, 2024 •
edited
Loading

Summary of the Pull Request

Split from #32351
Closely linked to #32514 and #32519.

PR Checklist

  • Closes: "Open", "Launch", "Go to"... #30685
  • Communication: I've discussed this with core contributors already. If work hasn't been agreed, this work might be rejected
  • Localization: All end user facing strings can be localized
  • Dev docs: Added/updated

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Build

This was referenced Apr 18, 2024
Closed
Closed
@Jay-o-Way Jay-o-Way marked this pull request as ready for review April 18, 2024 13:17
@Jay-o-Way Jay-o-Way added this to the PowerToys 0.81 milestone Apr 18, 2024
@Jay-o-Way Jay-o-Way changed the title Launch open go: Code Launch/Open/GoTo: Code Apr 22, 2024
jaimecbernardo
jaimecbernardo requested changes May 29, 2024
View reviewed changes
Copy link
Collaborator

@jaimecbernardo jaimecbernardo left a comment

Choose a reason for hiding this comment

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

Thanks for opening the PR.
I see the PR will contain changes that seem to be changing localization ids, retriggering the localization for all these strings.
It also changes names of telemetry events, which means it'll cause confusion in the names of events received and they'll be treated like two different events across versions.
I paused my review, since the PR seems to have many of these. These are not visible for the user, so can we please not make these changes as they'll cause more work / confusion for changes that won't affect the user in any way?

src/modules/Hosts/Hosts/HostsXAML/App.xaml.cs Outdated
/// <summary>
/// Invoked when the application is launched.
/// </summary>
/// <param name="args">Details about the launch request and process.</param>
protected override void OnLaunched(LaunchActivatedEventArgs args)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we deleting these comments? 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Uhm, I have forgotten now. But I would guess that this comment is so obvious?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please let's keep the changes at the scope of the PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Reverted ✔️

src/modules/fancyzones/FancyZonesLib/Resources.resx Outdated
@@ -183,17 +183,17 @@
<data name="Setting_Description_RestoreSize" xml:space="preserve">
<value>Restore the original size of windows when unsnapping</value>
</data>
<data name="Setting_Launch_Editor_Label" xml:space="preserve">
Copy link
Collaborator

Choose a reason for hiding this comment

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

These are internal labels which will retrigger localization. I don't think it's worth retriggering localization for all these strings just to change internal names.

Copy link
Collaborator Author

@Jay-o-Way Jay-o-Way May 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hm, I see the point. Reverted ✔️

Copy link
Collaborator Author

@Jay-o-Way Jay-o-Way May 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

LOL this entire file seems to be obsolete
@SeraphimaZykova can you confirm?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, please revert the labels. Revert the changes from xaml / resource getting in code in case those are changed as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Already done after your comment, check out the latest commits :)

src/modules/fancyzones/FancyZonesLib/trace.cpp Outdated
@@ -16,7 +16,7 @@
#define EventEnableFancyZonesKey "FancyZones_EnableFancyZones"
#define EventKeyDownKey "FancyZones_OnKeyDown"
#define EventZoneSettingsChangedKey "FancyZones_ZoneSettingsChanged"
#define EventEditorLaunchKey "FancyZones_EditorLaunch"
#define EventEditorOpenKey "FancyZones_EditorOpen"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will make old events not match new ones. It doesn't seem to make sense.

Copy link
Collaborator Author

@Jay-o-Way Jay-o-Way May 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I see. I'm not familiar with telemetry. Reverted ✔️

Copy link
Collaborator

Choose a reason for hiding this comment

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

So: revert any/all trace.cpp and trace.h files, right?

Yes, please :) And the calls to those as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Already done after your comment, check out the latest commits :)

Jay-o-Way
Jay-o-Way commented May 29, 2024
View reviewed changes
src/settings-ui/Settings.UI/SettingsXAML/OOBE/Views/OobeRegistryPreview.xaml.cs Outdated
@@ -27,10 +27,10 @@ public OobeRegistryPreview()

private void Launch_RegistryPreview_Click(object sender, Microsoft.UI.Xaml.RoutedEventArgs e)
{
ShellPage.SendDefaultIPCMessage("{\"action\":{\"RegistryPreview\":{\"action_name\":\"Launch\", \"value\":\"\"}}}");
ShellPage.SendDefaultIPCMessage("{\"action\":{\"RegistryPreview\":{\"action_name\":\"Open\", \"value\":\"\"}}}");
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jaimecbernardo I wonder if this will be a "breaking change" too?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You're right, it might be 🤔 Let's leave it as launch, I guess?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Already done after your comment, check out the latest commits :)

Jay-o-Way
Jay-o-Way commented May 29, 2024
View reviewed changes
src/modules/fancyzones/FancyZonesLib/Resources.resx Outdated
@@ -183,17 +183,17 @@
<data name="Setting_Description_RestoreSize" xml:space="preserve">
<value>Restore the original size of windows when unsnapping</value>
</data>
<data name="Setting_Launch_Editor_Label" xml:space="preserve">
Copy link
Collaborator Author

@Jay-o-Way Jay-o-Way May 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hm, I see the point. Reverted ✔️

src/modules/fancyzones/FancyZonesLib/SettingsConstants.h Outdated Show resolved Hide resolved
src/modules/fancyzones/FancyZonesLib/trace.cpp Outdated
@@ -16,7 +16,7 @@
#define EventEnableFancyZonesKey "FancyZones_EnableFancyZones"
#define EventKeyDownKey "FancyZones_OnKeyDown"
#define EventZoneSettingsChangedKey "FancyZones_ZoneSettingsChanged"
#define EventEditorLaunchKey "FancyZones_EditorLaunch"
#define EventEditorOpenKey "FancyZones_EditorOpen"
Copy link
Collaborator Author

@Jay-o-Way Jay-o-Way May 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I see. I'm not familiar with telemetry. Reverted ✔️

@Jay-o-Way Jay-o-Way removed this from the PowerToys 0.81 milestone May 29, 2024
@Jay-o-Way Jay-o-Way requested a review from jaimecbernardo May 31, 2024 13:11
@Jay-o-Way Jay-o-Way requested review from jaimecbernardo and removed request for jaimecbernardo July 4, 2024 23:17
@Jay-o-Way Jay-o-Way requested review from crutkas and ethanfangg August 1, 2024 11:49
@Jay-o-Way
Copy link
Collaborator Author

helloooo???

@Jay-o-Way Jay-o-Way closed this by deleting the head repository Oct 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaimecbernardo jaimecbernardo Awaiting requested review from jaimecbernardo

@crutkas crutkas Awaiting requested review from crutkas

@ethanfangg ethanfangg Awaiting requested review from ethanfangg

Assignees
No one assigned
Labels
revisit-0.82.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Jay-o-Way @jaimecbernardo