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

Use the tab's active title for Export Text #13915

Merged
3 commits merged into from
Sep 7, 2022
Merged

Use the tab's active title for Export Text #13915

3 commits merged into from
Sep 7, 2022

Conversation

serd2011
Copy link
Contributor

@serd2011 serd2011 commented Sep 2, 2022 •
edited by DHowett
Loading

Takes title from the tab instead of the TermControl

Closes #13909

@ghost ghost added Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. labels Sep 2, 2022
@DHowett DHowett changed the title Export Text feature uses tab's title even if it was changed Use the tab's active title for Export Text Sep 3, 2022
DHowett
DHowett reviewed Sep 3, 2022
View reviewed changes
src/cascadia/TerminalApp/TabManagement.cpp Outdated
@@ -420,7 +420,7 @@ namespace winrt::TerminalApp::implementation
// GH#11356 - we can't use the UWP apis for writing the file,
// because they don't work elevated (shocker) So just use the
// shell32 file picker manually.
path = co_await SaveFilePicker(*_hostingHwnd, [control](auto&& dialog) {
path = co_await SaveFilePicker(*_hostingHwnd, [&tab](auto&& dialog) {
Copy link
Member

Choose a reason for hiding this comment

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

We may actually want to capture tab by copy, if we can. It will increment a reference to the underlying smart pointer.

Capturing a reference with & during a coroutine handoff can in some cases result in the function blowing up later when it refers to the old coroutine's stack location. It can be messy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy constructor for TerminalTab is implicitly deleted cuz deep down winrt::impl::root_implements has std::atomic in it which is not CopyConstructible.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I didn't realize it was a type from the implementation namespace. Sorry!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, since all we need from tab is it's title we can get it outside of coroutine and just pass wstring by value.

std::wstring filename{ tab.Title() };
filename = til::clean_filename(filename);
path = co_await SaveFilePicker(*_hostingHwnd, [filename](auto&& dialog) {

Will this be fine?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I love it! You can move the filename into the lambda as well!

DHowett
DHowett requested changes Sep 3, 2022
View reviewed changes
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

(noted)

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 3, 2022
@DHowett
Copy link
Member

DHowett commented Sep 3, 2022

As always, thanks for tackling these things! :)

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 3, 2022
@serd2011 serd2011 requested a review from DHowett September 5, 2022 18:06
DHowett
DHowett approved these changes Sep 6, 2022
View reviewed changes
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

Thanks so much for doing this! I appreciate your response to our feedback, even (or especially!) when it is wrong. 😁

src/cascadia/TerminalApp/TabManagement.cpp Outdated Show resolved Hide resolved
@DHowett DHowett added the Needs-Second It's a PR that needs another sign-off label Sep 6, 2022
@serd2011 @DHowett
added suggestion
32c8f62 
Co-authored-by: Dustin L. Howett <dustin@howett.net>
@zadjii-msft zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Sep 7, 2022
@ghost
Copy link

ghost commented Sep 7, 2022

Hello @zadjii-msft!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 2c341c8 into microsoft:main Sep 7, 2022
DHowett pushed a commit that referenced this pull request Sep 9, 2022
@serd2011 @DHowett
Use the tab's active title for Export Text (#13915)
decfc3d 
Takes title from the tab instead of the TermControl

Closes #13909

(cherry picked from commit 2c341c8)
Service-Card-Id: 85427880
Service-Version: 1.15
@ghost
Copy link

ghost commented Sep 13, 2022

🎉Windows Terminal v1.15.252 has been released which incorporates this pull request.:tada:

Handy links:

@ghost ghost mentioned this pull request Sep 13, 2022
Closed
@ghost
Copy link

ghost commented Sep 13, 2022

🎉Windows Terminal Preview v1.16.252 has been released which incorporates this pull request.:tada:

Handy links:

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zadjii-msft zadjii-msft zadjii-msft approved these changes

@DHowett DHowett DHowett approved these changes

@PankajBhojwani PankajBhojwani Awaiting requested review from PankajBhojwani

@carlos-zamora carlos-zamora Awaiting requested review from carlos-zamora

@lhecker lhecker Awaiting requested review from lhecker

Assignees
No one assigned
Labels
Area-UserInterface Issues pertaining to the user interface of the Console or Terminal AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Second It's a PR that needs another sign-off Priority-3 A description (P3) Product-Terminal The new Windows Terminal.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

If you rename the tab and then click on Export Text, the filename is the previous title of the tab
3 participants
@serd2011 @DHowett @zadjii-msft