This one came from downstream bugzilla. Let's check the validity of formula expression input in the conditional formatting dialog, and give the users some feedback as appropriate. We already do that in several other dialogs such as the pivot table dialog, named range dialog, and the chart dialog.
I'll try this for 3.6.
Adding Markus in CC since he's reworking the conditional formatting dialog.
This is trivial and I'll add it for the new dialog. I think it is enough to use the highlight color for the input box and write a warning message in the first line: The line would like: Condition 2 The formula is invalid.
I'll re-assign it to Markus per comment 3.
I have a simple solution for it by highlighting the edit field if the formula is invalid which I currently can not push because we have a small problem with the drawing code of an edit if the background color is set.
I have finally a patch for the problem with the background color of the edit. It seems that we painted the border before the background color if we used native borders which resulted in the background color drawing over the borders.
Markus Mohrhard committed a patch related to this issue. It has been pushed to "master": http://cgit.freedesktop.org/libreoffice/core/commit/?id=76ccb5fe45aef020f51a6c0d5ed14c95390a471f highlight invalid formulas in cond format dlg, fdo#43653, bnc#730048
@Kohei: Could you check if that is what you had in mind?
Yup, it is what I had in mind. Looks good to me. :-) Let me just throw 2 things that came to mind. 1) The color is different from the ones used in other dialogs i.e. the chart dialog and the main pivot table dialog. Those two use a reddish pink color whereas yours uses (in my desktop) blue. I have no opinion which color is better, but we should probably standardized on one color. In the other two places the colors are hard-coded, so maybe we should switch them to using GetHighlightColor() to make it theme-able. 2) The formula expression checker is not very strict. For instance, when I type the following correct formula expression ISEVEN(ROW()) then immediately type ')' at the end, the background color turns blue. This is good. But if you type anything else, it won't flag it as incorrect, and the color remains white... For instance, the following expression are technically incorrect, but the background color remains white. ISEVEN(ROW() ISEVEN(ROW())asdf (ISEVEN(ROW()) LEN('text') (<-literals must be double-quoted) I know you used ScCompiler to check formula expression's validity, so this may be just a known limitation of the compiler. I'm not sure if there is any way to make it more strict, using ScCompiler alone... If there isn't, then we can just leave it as-is for now, and make this a future project.
(In reply to comment #9) > Yup, it is what I had in mind. Looks good to me. :-) > > Let me just throw 2 things that came to mind. > > 1) The color is different from the ones used in other dialogs i.e. the chart > dialog and the main pivot table dialog. Those two use a reddish pink color > whereas yours uses (in my desktop) blue. I have no opinion which color is > better, but we should probably standardized on one color. In the other two > places the colors are hard-coded, so maybe we should switch them to using > GetHighlightColor() to make it theme-able. I'll cc Astron for this one. Personally I'm not happy with any of the current solutions. The highlight color looks strange for error cases and the hard coded colors are not perfect in a themed LibO: Maybe we need a themed warning and error color. > > 2) The formula expression checker is not very strict. For instance, when I > type the following correct formula expression > > ISEVEN(ROW()) > > then immediately type ')' at the end, the background color turns blue. This is > good. But if you type anything else, it won't flag it as incorrect, and the > color remains white... For instance, the following expression are technically > incorrect, but the background color remains white. > > ISEVEN(ROW() We may change this one with the autocorrection and autoclose brackets flag in the compiler. We could also use another color as long as the compiler can correct the formula but the formula itself would not be valid. Maybe we should just ask the UX guys here. > ISEVEN(ROW())asdf no idea about that one > (ISEVEN(ROW()) same as the first case > LEN('text') (<-literals must be double-quoted) I wonder why the compiler does not flag this one. > > I know you used ScCompiler to check formula expression's validity, so this may > be just a known limitation of the compiler. I'm not sure if there is any way > to make it more strict, using ScCompiler alone... If there isn't, then we can > just leave it as-is for now, and make this a future project. Do you have a better idea than using ScCompiler. IMHO creating a ScFormulaCell for this case might find more errors for the price of a bit more complex handling in the code.
(In reply to comment #10) > Do you have a better idea than using ScCompiler. IMHO creating a ScFormulaCell > for this case might find more errors for the price of a bit more complex > handling in the code. Well, despite the limitations I still think using ScCompiler would be our best choice, though we would need to modify it to make it more strict. I'd rather not use a cell class here since we plan to eliminate class objects in the future. We might be able to re-use ScInterpreter to check grammar errors, but that's just a random idea. Let me keep thinking about it.
Hi there. Sorry for the lag. Okay, so as to the colour: I'd personally prefer a hard-coded red over just using the selection colour. That said, it's obviously not ideal and some people on XP/Linux might even use themes whose selection colour is actually red already (thus clashing). Still, for Mac/Windows Vista/7/8 users, red should work nearly everywhere. I am unaware of existing system defaults (there may be for GTK+ and Qt... don't know, can't imagine that there is one for Windows), if such a default exists, it should of course be used. In any case, to standardise the colours, maybe you can define a colour COLOR_ERROR_BACKGROUND or so, so we can do a quick switcheroo when the occasion arises (or maybe native toolkits do theme this and then we could quickly use this colour). I'll have a look at how it's actually implemented so far, if/when I finally have a current build again.
Ok, I switched the Color to COL_RED but will msot likely switch to COL_LIGHTRED soon. We may consider to backport it like it is. It is still an improvement to the existing situation. I also talked to Eike about using ScCompiler + ScInterpreter to check formulas in dialogs and while it should be generally possible we may need to add some checks in ScCompiler in the matrix and dde code to handle the places where ScFormulaCell is used.
Switched to COL_LIGHTRED which looks much more like an error color. I played a bit with the different setting of ScCompiler to make the check stricter but it seems it does not work in this case. All in all the solution we need to implement is to make ScInterpreter useable without a ScFormulaCell. This requires some effort that should not be tracked by this bug so I'm closing for now.