Merge remote-tracking branch 'origin/main' into dev/migrie/f/local-sn…

…ippets-cleaner

src/buffer/out/UTextAdapter.cpp

@@ -186,23 +186,22 @@ catch (...)
 static UBool U_CALLCONV utextAccess(UText* ut, int64_t nativeIndex, UBool forward) noexcept
 try
 {
- if (nativeIndex < 0)
- {
- nativeIndex = 0;
- }
-
  auto neededIndex = nativeIndex;
+ // This will make it simpler for us to search the row that contains the nativeIndex,
+ // because we'll now only need to check for `start<=index<limit` and nothing else.
  if (!forward)
  {
  neededIndex--;
  }
 
  const auto& textBuffer = *static_cast<const TextBuffer*>(ut->context);
  const auto range = accessRowRange(ut);
- auto start = ut->chunkNativeStart;
- auto limit = ut->chunkNativeLimit;
+ const auto startOld = ut->chunkNativeStart;
+ const auto limitOld = ut->chunkNativeLimit;
+ auto start = startOld;
+ auto limit = limitOld;
 
- if (neededIndex < start || neededIndex >= limit)
+ if (neededIndex < startOld || neededIndex >= limitOld)
  {
  auto y = accessCurrentRow(ut);
  std::wstring_view text;
@@ -215,8 +214,7 @@ try
  --y;
  if (y < range.begin)
  {
- assert(false);
- return false;
+ break;
  }
 
  const auto& row = textBuffer.GetRowByOffset(y);
@@ -235,8 +233,7 @@ try
  ++y;
  if (y >= range.end)
  {
- assert(false);
- return false;
+ break;
  }
 
  const auto& row = textBuffer.GetRowByOffset(y);
@@ -249,38 +246,50 @@ try
  } while (neededIndex >= limit);
  }
 
- if (!wasWrapForced)
+ assert(start >= 0);
+ // If we have already calculated the total length we can also assert that the limit is in range.
+ assert(ut->p == nullptr || static_cast<size_t>(limit) <= accessLength(ut));
+
+ // Even if we went out-of-bounds, we still need to update the chunkContents to contain the first/last chunk.
+ if (limit != limitOld)
  {
- const auto newSize = text.size() + 1;
- const auto buffer = RefcountBuffer::EnsureCapacityForOverwrite(accessBuffer(ut), newSize);
+ if (!wasWrapForced)
+ {
+ const auto newSize = text.size() + 1;
+ const auto buffer = RefcountBuffer::EnsureCapacityForOverwrite(accessBuffer(ut), newSize);
 
- memcpy(&buffer->data[0], text.data(), text.size() * sizeof(wchar_t));
- til::at(buffer->data, text.size()) = L'\n';
+  memcpy(&buffer->data[0], text.data(), text.size() * sizeof(wchar_t));
+  til::at(buffer->data, text.size()) = L'\n';
 
- text = { &buffer->data[0], newSize };
- accessBuffer(ut) = buffer;
- }
+  text = { &buffer->data[0], newSize };
+  accessBuffer(ut) = buffer;
+  }
 
- accessCurrentRow(ut) = y;
- ut->chunkNativeStart = start;
- ut->chunkNativeLimit = limit;
- ut->chunkLength = gsl::narrow_cast<int32_t>(text.size());
+  accessCurrentRow(ut) = y;
+  ut->chunkNativeStart = start;
+  ut->chunkNativeLimit = limit;
+  ut->chunkLength = gsl::narrow_cast<int32_t>(text.size());
 #pragma warning(suppress : 26490) // Don't use reinterpret_cast (type.1).
- ut->chunkContents = reinterpret_cast<const char16_t*>(text.data());
- ut->nativeIndexingLimit = ut->chunkLength;
+ ut->chunkContents = reinterpret_cast<const char16_t*>(text.data());
+ ut->nativeIndexingLimit = ut->chunkLength;
+ }
  }
 
- auto offset = gsl::narrow_cast<int32_t>(nativeIndex - start);
-
+ // The ICU documentation is a little bit misleading. It states:
+ // > @param forward [...] If true, start<=index<limit. If false, [...] start<index<=limit.
+ // but that's just for finding the target chunk. The chunkOffset is not actually constrained to that!
+ // std::clamp will perform a<=b<=c, which is what we want.
+ const auto clampedIndex = std::clamp(nativeIndex, start, limit);
+ auto offset = gsl::narrow_cast<int32_t>(clampedIndex - start);
  // Don't leave the offset on a trailing surrogate pair. See U16_SET_CP_START.
  // This assumes that the TextBuffer contains valid UTF-16 which may theoretically not be the case.
  if (offset > 0 && offset < ut->chunkLength && U16_IS_TRAIL(til::at(ut->chunkContents, offset)))
  {
  offset--;
  }
-
  ut->chunkOffset = offset;
- return true;
+
+ return neededIndex >= start && neededIndex < limit;
 }
 catch (...)
 {

src/buffer/out/textBuffer.cpp

@@ -2790,7 +2790,7 @@ void TextBuffer::Serialize(const wchar_t* destination) const
  // In other words, we can only skip \x1b[K = Erase in Line, if both the first/last attribute are the default attribute.
  static constexpr TextAttribute defaultAttr;
  const auto trimTrailingWhitespaces = it == last && lastCharX < newX;
- const auto clearToEndOfLine = trimTrailingWhitespaces && beg->value != defaultAttr || beg->value != defaultAttr;
+ const auto clearToEndOfLine = trimTrailingWhitespaces && (beg->value != defaultAttr || last->value != defaultAttr);
 
  if (trimTrailingWhitespaces)
  {

src/cascadia/TerminalApp/Pane.cpp

@@ -27,10 +27,10 @@ static const int CombinedPaneBorderSize = 2 * PaneBorderSize;
 static const int AnimationDurationInMilliseconds = 200;
 static const Duration AnimationDuration = DurationHelper::FromTimeSpan(winrt::Windows::Foundation::TimeSpan(std::chrono::milliseconds(AnimationDurationInMilliseconds)));
 
-Pane::Pane(const IPaneContent& content, const bool lastFocused) :
- _content{ content },
+Pane::Pane(IPaneContent content, const bool lastFocused) :
  _lastActive{ lastFocused }
 {
+ _setPaneContent(std::move(content));
  _root.Children().Append(_borderFirst);
 
  const auto& control{ _content.GetRoot() };
@@ -985,17 +985,7 @@ void Pane::_ContentLostFocusHandler(const winrt::Windows::Foundation::IInspectab
 // - <none>
 void Pane::Close()
 {
- // Pane has two events, CloseRequested and Closed. CloseRequested is raised by the content asking to be closed,
- // but also by the window who owns the tab when it's closing. The event is then caught by the TerminalTab which
- // calls Close() which then raises the Closed event. Now, if this is the last pane in the window, this will result
- // in the window raising CloseRequested again which leads to infinite recursion, so we need to guard against that.
- // Ideally we would have just a single event in the future.
- if (_closed)
- {
- return;
- }
-
- _closed = true;
+ _setPaneContent(nullptr);
  // Fire our Closed event to tell our parent that we should be removed.
  Closed.raise(nullptr, nullptr);
 }
@@ -1007,7 +997,7 @@ void Pane::Shutdown()
 {
  if (_IsLeaf())
  {
- _content.Close();
+ _setPaneContent(nullptr);
  }
  else
  {
@@ -1411,7 +1401,7 @@ void Pane::_CloseChild(const bool closeFirst)
  _borders = _GetCommonBorders();
 
  // take the control, profile, id and isDefTermSession of the pane that _wasn't_ closed.
- _content = remainingChild->_content;
+ _setPaneContent(remainingChild->_takePaneContent());
  _id = remainingChild->Id();
 
  // Revoke the old event handlers. Remove both the handlers for the panes
@@ -1716,6 +1706,34 @@ void Pane::_SetupChildCloseHandlers()
  });
 }
 
+// With this method you take ownership of the control from this Pane.
+// Assign it to another Pane with _setPaneContent() or Close() it.
+IPaneContent Pane::_takePaneContent()
+{
+ _closeRequestedRevoker.revoke();
+ return std::move(_content);
+}
+
+// This method safely sets the content of the Pane. It'll ensure to revoke and
+// assign event handlers, and to Close() the existing content if there's any.
+// The new content can be nullptr to remove any content.
+void Pane::_setPaneContent(IPaneContent content)
+{
+ // The IPaneContent::Close() implementation may be buggy and raise the CloseRequested event again.
+ // _takePaneContent() avoids this as it revokes the event handler.
+ if (const auto c = _takePaneContent())
+ {
+ c.Close();
+ }
+
+ _content = std::move(content);
+
+ if (_content)
+ {
+ _closeRequestedRevoker = _content.CloseRequested(winrt::auto_revoke, [this](auto&&, auto&&) { Close(); });
+ }
+}
+
 // Method Description:
 // - Sets up row/column definitions for this pane. There are three total
 // row/cols. The middle one is for the separator. The first and third are for
@@ -2266,8 +2284,7 @@ std::pair<std::shared_ptr<Pane>, std::shared_ptr<Pane>> Pane::_Split(SplitDirect
  else
  {
  // Move our control, guid, isDefTermSession into the first one.
- _firstChild = std::make_shared<Pane>(_content);
- _content = nullptr;
+ _firstChild = std::make_shared<Pane>(_takePaneContent());
  _firstChild->_broadcastEnabled = _broadcastEnabled;
  }
 
@@ -2462,6 +2479,11 @@ bool Pane::_HasChild(const std::shared_ptr<Pane> child)
  });
 }
 
+winrt::TerminalApp::TerminalPaneContent Pane::_getTerminalContent() const
+{
+ return _IsLeaf() ? _content.try_as<winrt::TerminalApp::TerminalPaneContent>() : nullptr;
+}
+
 // Method Description:
 // - Recursive function that finds a pane with the given ID
 // Arguments:

src/cascadia/TerminalApp/Pane.h

@@ -62,7 +62,7 @@ struct PaneResources
 class Pane : public std::enable_shared_from_this<Pane>
 {
 public:
- Pane(const winrt::TerminalApp::IPaneContent& content,
+ Pane(winrt::TerminalApp::IPaneContent content,
  const bool lastFocused = false);
 
  Pane(std::shared_ptr<Pane> first,
@@ -248,13 +248,13 @@ class Pane : public std::enable_shared_from_this<Pane>
 
  std::optional<uint32_t> _id;
  std::weak_ptr<Pane> _parentChildPath{};
- bool _closed{ false };
  bool _lastActive{ false };
  winrt::event_token _firstClosedToken{ 0 };
  winrt::event_token _secondClosedToken{ 0 };
 
  winrt::Windows::UI::Xaml::UIElement::GotFocus_revoker _gotFocusRevoker;
  winrt::Windows::UI::Xaml::UIElement::LostFocus_revoker _lostFocusRevoker;
+ winrt::TerminalApp::IPaneContent::CloseRequested_revoker _closeRequestedRevoker;
 
  Borders _borders{ Borders::None };
 
@@ -264,11 +264,10 @@ class Pane : public std::enable_shared_from_this<Pane>
  bool _IsLeaf() const noexcept;
  bool _HasFocusedChild() const noexcept;
  void _SetupChildCloseHandlers();
+ winrt::TerminalApp::IPaneContent _takePaneContent();
+ void _setPaneContent(winrt::TerminalApp::IPaneContent content);
  bool _HasChild(const std::shared_ptr<Pane> child);
- winrt::TerminalApp::TerminalPaneContent _getTerminalContent() const
- {
- return _IsLeaf() ? _content.try_as<winrt::TerminalApp::TerminalPaneContent>() : nullptr;
- }
+ winrt::TerminalApp::TerminalPaneContent _getTerminalContent() const;
 
  std::pair<std::shared_ptr<Pane>, std::shared_ptr<Pane>> _Split(winrt::Microsoft::Terminal::Settings::Model::SplitDirection splitType,
  const float splitSize,

src/cascadia/TerminalApp/ScratchpadContent.cpp

@@ -45,7 +45,6 @@ namespace winrt::TerminalApp::implementation
  }
  void ScratchpadContent::Close()
  {
- CloseRequested.raise(*this, nullptr);
  }
 
  INewContentArgs ScratchpadContent::GetNewTerminalArgs(const BuildStartupKind /* kind */) const

src/cascadia/TerminalApp/SettingsPaneContent.cpp

@@ -47,7 +47,6 @@ namespace winrt::TerminalApp::implementation
  }
  void SettingsPaneContent::Close()
  {
- CloseRequested.raise(*this, nullptr);
  }
 
  INewContentArgs SettingsPaneContent::GetNewTerminalArgs(const BuildStartupKind /*kind*/) const

src/cascadia/TerminalApp/TerminalPaneContent.cpp

@@ -78,8 +78,6 @@ namespace winrt::TerminalApp::implementation
  _bellPlayer = nullptr;
  _bellPlayerCreated = false;
  }
-
- CloseRequested.raise(*this, nullptr);
  }
 
  winrt::hstring TerminalPaneContent::Icon() const
@@ -239,19 +237,20 @@ namespace winrt::TerminalApp::implementation
 
  if (_profile)
  {
- if (_isDefTermSession && _profile.CloseOnExit() == CloseOnExitMode::Automatic)
- {
- // For 'automatic', we only care about the connection state if we were launched by Terminal
- // Since we were launched via defterm, ignore the connection state (i.e. we treat the
- // close on exit mode as 'always', see GH #13325 for discussion)
- Close();
- }
-
  const auto mode = _profile.CloseOnExit();
- if ((mode == CloseOnExitMode::Always) ||
- ((mode == CloseOnExitMode::Graceful || mode == CloseOnExitMode::Automatic) && newConnectionState == ConnectionState::Closed))
+
+ if (
+ // This one is obvious: If the user asked for "always" we do just that.
+ (mode == CloseOnExitMode::Always) ||
+ // Otherwise, and unless the user asked for the opposite of "always",
+ // close the pane when the connection closed gracefully (not failed).
+ (mode != CloseOnExitMode::Never && newConnectionState == ConnectionState::Closed) ||
+ // However, defterm handoff can result in Windows Terminal randomly opening which may be annoying,
+ // so by default we should at least always close the pane, even if the command failed.
+ // See GH #13325 for discussion.
+ (mode == CloseOnExitMode::Automatic && _isDefTermSession))
  {
- Close();
+ CloseRequested.raise(nullptr, nullptr);
  }
  }
  }
@@ -331,7 +330,7 @@ namespace winrt::TerminalApp::implementation
  void TerminalPaneContent::_closeTerminalRequestedHandler(const winrt::Windows::Foundation::IInspectable& /*sender*/,
  const winrt::Windows::Foundation::IInspectable& /*args*/)
  {
- Close();
+ CloseRequested.raise(nullptr, nullptr);
  }
 
  void TerminalPaneContent::_restartTerminalRequestedHandler(const winrt::Windows::Foundation::IInspectable& /*sender*/,

src/cascadia/TerminalApp/TerminalTab.cpp

@@ -947,26 +947,6 @@ namespace winrt::TerminalApp::implementation
  auto dispatcher = TabViewItem().Dispatcher();
  ContentEventTokens events{};
 
- events.CloseRequested = content.CloseRequested(
- winrt::auto_revoke,
- [this](auto&& sender, auto&&) {
- if (const auto content{ sender.try_as<TerminalApp::IPaneContent>() })
- {
- // Calling Close() while walking the tree is not safe, because Close() mutates the tree.
- const auto pane = _rootPane->_FindPane([&](const auto& p) -> std::shared_ptr<Pane> {
- if (p->GetContent() == content)
- {
- return p;
- }
- return {};
- });
- if (pane)
- {
- pane->Close();
- }
- }
- });
-
  events.TitleChanged = content.TitleChanged(
  winrt::auto_revoke,
  [dispatcher, weakThis](auto&&, auto&&) -> winrt::fire_and_forget {

src/cascadia/TerminalApp/TerminalTab.h

@@ -134,7 +134,6 @@ namespace winrt::TerminalApp::implementation
  winrt::TerminalApp::IPaneContent::ConnectionStateChanged_revoker ConnectionStateChanged;
  winrt::TerminalApp::IPaneContent::ReadOnlyChanged_revoker ReadOnlyChanged;
  winrt::TerminalApp::IPaneContent::FocusRequested_revoker FocusRequested;
- winrt::TerminalApp::IPaneContent::CloseRequested_revoker CloseRequested;
 
  // These events literally only apply if the content is a TermControl.
  winrt::Microsoft::Terminal::Control::TermControl::KeySent_revoker KeySent;

src/cascadia/TerminalConnection/ConptyConnection.cpp

@@ -431,12 +431,10 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
  try
  {
  // GH#11556 - make sure to format the error code to this string as an UNSIGNED int
- winrt::hstring exitText{ fmt::format(std::wstring_view{ RS_(L"ProcessExited") }, fmt::format(_errorFormat, status)) };
- TerminalOutput.raise(L"\r\n");
- TerminalOutput.raise(exitText);
- TerminalOutput.raise(L"\r\n");
- TerminalOutput.raise(RS_(L"CtrlDToClose"));
- TerminalOutput.raise(L"\r\n");
+ const auto msg1 = fmt::format(std::wstring_view{ RS_(L"ProcessExited") }, fmt::format(_errorFormat, status));
+ const auto msg2 = RS_(L"CtrlDToClose");
+ const auto msg = fmt::format(FMT_COMPILE(L"\r\n{}\r\n{}\r\n"), msg1, msg2);
+ TerminalOutput.raise(msg);
  }
  CATCH_LOG();
  }