-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Introduce a ContentManager helper #14851
Changes from 1 commit
581acd4
a5255ba
e6220b7
936c01f
439b21f
99bc280
2195515
5116ca1
ef7e2ed
af14c2b
d456210
f5b030c
2c4613a
7a3e2e0
f655296
274d62d
e623299
1b4f1ef
47336c0
9924e23
f06e484
2822c36
f27db59
2332f0c
4e7da2e
23c4d4c
761bd6a
93e9dc5
d5396d1
07ff418
40fdbc1
4db381e
64257d8
3026922
84e228f
dc1ae9a
8bb8391
0f4c4d8
a5a9930
6e6d14e
3fb8e8c
4d5f6d2
a7379ca
0b79e81
950ce6c
055da35
b0726c2
e214624
c69f0bc
7e91bdb
0395dc4
f904e5d
4548729
7660937
0ad5b59
cfa6108
e40575b
dffb416
a769933
c065897
33685d9
a4f19a9
1ec8c0d
2621519
603a2ce
a9ac218
82224bc
d9d4d2e
3f9deca
0f0316f
7734600
2873511
855a79d
4b22963
d06ad8b
118bffa
aa8b0c5
bc80943
667b658
2bc578b
62a4ee0
091f32c
ecab57f
13257da
f9caf19
9a47396
273f2a8
c79f27c
b75fb24
cacb822
6934bc8
b589d09
0199aba
5e23a72
ada3f42
5164bff
7e2eb0d
5c852c3
1e331a0
8e2c7a7
08f6a53
8803324
b983c69
48aa555
3aa083b
9a9fa43
11ef8ef
b24cf61
3fa1017
5fe3fa5
b0ca581
74af809
547b2c9
6f6880c
1dc2436
6e4b2e1
7721813
6dead99
434abc2
1b59eb9
39a9450
44b238e
073350e
ca511c9
f70775a
81140a5
339972e
6d04353
1138416
9957e5c
c8ce5b4
6aec80b
9316f5b
0808f94
5b3dc08
59b07f1
7142ae8
d55bb43
11f9957
55ee49b
fe2ee91
1e4cc33
5a420d8
24da827
b02b935
f73ec9a
3659744
3af4d94
697b420
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,26 +19,21 @@ using namespace winrt::Microsoft::Terminal; | |
using namespace winrt::Microsoft::Terminal::Control; | ||
using namespace winrt::Microsoft::Terminal::Settings::Model; | ||
|
||
namespace winrt | ||
{ | ||
namespace MUX = Microsoft::UI::Xaml; | ||
using IInspectable = Windows::Foundation::IInspectable; | ||
} | ||
namespace winrt::TerminalApp::implementation | ||
{ | ||
ControlInteractivity ContentManager::CreateCore(Microsoft::Terminal::Control::IControlSettings settings, | ||
IControlAppearance unfocusedAppearance, | ||
TerminalConnection::ITerminalConnection connection) | ||
ControlInteractivity ContentManager::CreateCore(const Microsoft::Terminal::Control::IControlSettings& settings, | ||
const IControlAppearance& unfocusedAppearance, | ||
const TerminalConnection::ITerminalConnection& connection) | ||
{ | ||
auto content = ControlInteractivity{ settings, unfocusedAppearance, connection }; | ||
ControlInteractivity content{ settings, unfocusedAppearance, connection }; | ||
content.Closed({ get_weak(), &ContentManager::_closedHandler }); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. qq on weak: is the content manager going away There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But is there any benefit to using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm cool with leaning towards safety here. Who knows, maybe there's a "the window closes, so we start tearing down, but the controls only get released after the rest of the app does" situation. I suppose that's covered by our There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's all fair; there is some overhead inherent in |
||
|
||
_content.emplace(content.Id(), content); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't love that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a way to scope this knowledge to ContentManager itself? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean, we could stash the id just within ContentManager. But then, when we want to detach content, we'd have to go reverse lookup the key for the content we want to detach. Sure that's just an O(N=number of controls) lookup, but that still feels blechy |
||
|
||
return content; | ||
} | ||
|
||
ControlInteractivity ContentManager::LookupCore(uint64_t id) | ||
ControlInteractivity ContentManager::TryLookupCore(uint64_t id) | ||
{ | ||
const auto it = _content.find(id); | ||
return it != _content.end() ? it->second : ControlInteractivity{ nullptr }; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,7 +46,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation | |
_lastMouseClickPos{}, | ||
_selectionNeedsToBeCopied{ false } | ||
{ | ||
_id = ControlInteractivity::_nextId++; | ||
_id = _nextId.fetch_add(1, std::memory_order_relaxed); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Editor's note: you can have up to 18,446,744,073,709,551,615 terminal panes in the same process before this becomes a problem; it is likely that we will crash before you get to there :) |
||
|
||
_core = winrt::make_self<ControlCore>(settings, unfocusedAppearance, connection); | ||
} | ||
|
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.
Want to know something even more fun? Since these are in the same module, if you just do
TerminalApp::ContentManager _contentManager;
it will use a fast path to callwinrt::make<...>
directly. You can treat the projected type as "law"! I forgot about this until I saw it withmake
in.You don't need to make this change, I just think it's v.cool. Check it out:
In
Foo.g.cpp
It provides a linkable constructor for
TerminalApp::Foo
, and doesn't even generate the one that callsRoActivateInstance
inwinrt/whatever.h