Open mmeeks opened 9 months ago
Hello @mmeeks @pedropintosilva,
I explored a bit of the codebase and found that there are still some dialogs that are not asynchronous. However, I am confused about some things. Here are my questions:
Is doing a
->run
that should be converted to arunAsync
; as should the other instances of->run()
insc/source/ui/view/cellsh1.cxx
?
Calc->Format->Conditional->Date
. Am I right? Here, I wrote asynchronous code, but it wasn't hitting that part.if(bContainsCondFormat && !bCondFormatDlg && bContainsExistingCondFormat)
{
std::shared_ptr<weld::MessageDialog> xQueryBox(Application::CreateMessageDialog(pTabViewShell->GetFrameWeld(),
VclMessageType::Question, VclButtonsType::YesNo,
ScResId(STR_EDIT_EXISTING_COND_FORMATS)));
xQueryBox->set_default_response(RET_YES);
// TODO: here
xQueryBox->runAsync(xQueryBox, [this, &rCondFormats, pList, &pCondFormat, &nIndex, &bCondFormatDlg] (int nResult) mutable {
bool bEditExisting = nResult == RET_YES;
if (bEditExisting)
{
// differentiate between ranges where one conditional format is defined
// and several formats are defined
// if we have only one => open the cond format dlg to edit it
// otherwise open the manage cond format dlg
if (rCondFormats.size() == 1)
{
pCondFormat = pList->GetFormat(rCondFormats[0]);
assert(pCondFormat);
nIndex = pCondFormat->GetKey();
bCondFormatDlg = true;
}
else
{
// Queue message to open Conditional Format Manager Dialog.
GetViewData().GetDispatcher().Execute(SID_OPENDLG_CONDFRMT_MANAGER, SfxCallMode::ASYNCHRON);
}
}
else
{
// define an overlapping conditional format
pCondFormat = pList->GetFormat(rCondFormats[0]);
assert(pCondFormat);
bCondFormatDlg = true;
}
});
}
Similarly,
chart::ChartController::executeDialog_ObjectProperties_...
will need asyncing.
chart::ChartController::executeDialog_ObjectProperties_...
. Are you missing something?This might be useful: https://forum.collaboraonline.com/t/how-to-help-with-jsdialogs-development-porting-dialogs-to-native-html/
Hello @mmeeks @pedropintosilva,
- Which dialogs is this supposed to be for? I think it was hitting a breakpoint for
Calc->Format->Conditional->Date
. Am I right? Here, I wrote asynchronous code, but it wasn't hitting that part.
For this one you need an overlapping range with existing conditional formatting I think, so select A1:B1 (first two cells in first row) format, conditional, date, ok, then select A1:A2 (first two cells in first col), format, conditional, date: and now this message box about "The selected cell already contains conditional formatting..." should appear
Similarly,
chart::ChartController::executeDialog_ObjectProperties_...
will need asyncing.* I didn't find a reference for `chart::ChartController::executeDialog_ObjectProperties_...`. Are you missing something?
The case in https://user-images.githubusercontent.com/833656/285202358-fd1ef8d4-749b-4d96-a427-534e6ff0b4e6.svg is "Dlg" rather than "Dialog", specifically chart::ChartController::executeDlg_ObjectProperties_withoutUndoGuard in chart2/source/controller/main/ChartController_Properties.cxx
I have a PR falling in that category: https://github.com/CollaboraOnline/online/pull/7990
https://gerrit.libreoffice.org/c/core/+/161828 should be linked here. Its cool#1770 prefix is incorrect.
Known sync dialog:
WIP: I shall convert the 'slide properties dialog in impress' to async.
Added cool#7710: Make Slide Properties Dialog in Impress Async https://gerrit.libreoffice.org/c/core/+/171393
Probably more 'widgets' want to be transformed?
Added cool#7710: Make Slide Properties Dialog in Impress Async https://gerrit.libreoffice.org/c/core/+/171393
I assume other conversions done by other contributor will follow, hence I un-assign myself (again) having done mentioned commit earlier.
Impress Interaction dialog:
https://gerrit.libreoffice.org/c/core/+/171959
See #9836 for the command to enable it.
Calc profiling shows a couple of dialogs that are not async'd that end up being in-use; checkout the profile here:
https://user-images.githubusercontent.com/833656/285202358-fd1ef8d4-749b-4d96-a427-534e6ff0b4e6.svg
See the bits then end up in an ImplYield:: - these are not great.
ScCellShell::ExecuteEdit
is doing a '->run' that should be converted to a runAsync; as should the other instances of ->run() in sc/source/ui/view/cellsh1.cxx
Similarly - chart::ChartController::executeDialogObjectProperties... will need asyncing
I expect @caolanm could help with code-review. We will need to ensure each of these paths is tested - add a SAL_DEBUG("foo") to the core and ensure it has printed to verify each of them.
For sample code run a git log -- and see how some of the other call sites have moved to 'runAsync' successfully.
Thanks!