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

Only update the icon of a tab it the icon actually _changed_ #2376

Merged
merged 4 commits into from
Aug 14, 2019
Merged

Only update the icon of a tab it the icon actually _changed_ #2376

merged 4 commits into from
Aug 14, 2019

Conversation

zadjii-msft
Copy link
Member

@zadjii-msft zadjii-msft commented Aug 9, 2019 •
edited
Loading

Summary of the Pull Request

Only update the icon of a tab it the icon actually changed. The way we have it, we'll try and update the icon everytime focus changes, causing the tab icon to blink for a second, even when switching between two panes with the same profile.

References

While I was here, I also fixed #2329, I think accidentally. But that's cool!

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 9, 2019
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 9, 2019
DHowett-MSFT
DHowett-MSFT approved these changes Aug 14, 2019
View reviewed changes
Copy link
Contributor

@DHowett-MSFT DHowett-MSFT left a comment

Choose a reason for hiding this comment

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

Overall I still approve of this, but I want to understand why there's two copies of code from GetIconFromProfile (one in Tab, one in App)

@zadjii-msft zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Aug 14, 2019
@ghost
Copy link

ghost commented Aug 14, 2019

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.

@zadjii-msft zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Aug 14, 2019
DHowett-MSFT
DHowett-MSFT reviewed Aug 14, 2019
View reviewed changes
src/cascadia/TerminalApp/Tab.cpp
}

_tabViewItem.Icon(elem);
_tabViewItem.Icon(GetColoredIcon(_lastIconPath));
Copy link
Contributor

Choose a reason for hiding this comment

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

This one lost its exception handler.. It also no longer checks whether the path was empty. Does this work right for transitioning from "an icon" to "no icon"?

Copy link
Member Author

Choose a reason for hiding this comment

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

The exception handler went up into GetColoredIcon, but I'll double check the icon->null path

@DHowett-MSFT
Copy link
Contributor

How does this fix #2339?

@zadjii-msft
Copy link
Member Author

@DHowett-MSFT that was a typo, definitely meant #2329

@DHowett-MSFT
Copy link
Contributor

In that case, we can remove the exception handler around the otehr call. Do we need CATCH_LOG (it'll generate a report in the debugger)? or do we want a different type of catch (quieter kind)?

@DHowett-MSFT
Copy link
Contributor

It's baa~ack!

 fatal error C1083: Cannot open compiler intermediate file:
'd:\a\1\s\obj\x64\auditmode\localtests_terminalapp\terminalapp.localtests.pch': Not enough space

@DHowett-MSFT DHowett-MSFT merged commit 8999c66 into master Aug 14, 2019
@zadjii-msft zadjii-msft deleted the dev/migrie/b/1333-icon-flashing branch August 23, 2019 13:14
@ghost
Copy link

ghost commented Aug 27, 2019

🎉Windows Terminal Preview v0.4.2382.0 has been released which incorporates this pull request.:tada:

Handy links:

This was referenced Aug 27, 2019
Closed
Closed
@AdamDanischewski
Copy link

#2329 is not fixed in v0.4.2382.0

Try the repetition step listed in #2329. Looks like Windows Terminal could use a QA boost.

@Reelix
Copy link

Reelix commented Oct 3, 2019

Can confirm that #2329 is still broken in v0.5.2681.0

Simple Replication::
1.) Open profiles.json
2.) Change any profiles icon to

"icon": "InvalidFileName.png",

3.) Save

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DHowett-MSFT DHowett-MSFT DHowett-MSFT approved these changes

@carlos-zamora carlos-zamora carlos-zamora approved these changes

@miniksa miniksa Awaiting requested review from miniksa

Assignees
No one assigned
Labels
AutoMerge Marked for automatic merge by the bot when requirements are met Needs-Second It's a PR that needs another sign-off
Projects
None yet
Milestone
No milestone
5 participants
@zadjii-msft @DHowett-MSFT @AdamDanischewski @Reelix @carlos-zamora