I have run your test and in step 3
    Set "Cell value is" "equal to" = "success"
1. I have typed exacly "success" with " sign and your test fails (conditional formating works perfect)

2. I have typed just success (LibreOffice changes it to 'success') and your test passes (conditional formating is bad)

So I think it is connected how text value is treated, here changed to 'text' which is badly evaluated.

I think it is also connected to this Bug 42282

Yes, it works like Maciej Rumianowski wrote. I had test it in LibO3.5beta3

You have to write "success" and with quotation marks as value!

Hope i could help you.

This is an annoying one =) Neil just fell over this at Collabora; and I couldn't see what was wrong.

AFAICS if someone enters 'foo' in the entry instead of '"foo"' - we should warn them and/or (better) auto-correct their input to '"foo"' instead of 'foo' =)

I imagine that there is some over-complicated thing going on here - perhaps some ability to use defined names or abstract formulae or ... [ no idea ]. But for simple string cases we should definitely improve; fixing the help as per bug#42282 is only a partial solution.

sc/source/ui/condformat/condformatdlg.cxx

has the code for the conditional formatting dialog.

Thanks !

So I propose to just add a warning. The problem is that we allow formulas in this place and a string without "" is a column/range label which might break old systems if we just change it.
We are working on deprecating and possibly removing column and row labels but this will take quite some time. Until then I propose to just issue a warning that this results in something the user might not expect.

This has the additional benefit that we don't mess around with the formula compiler which is a huge pain in the long run.

Code pointers:

<moggi> [18:40:55] Renato_Ferreira: so there is a ScSimpleFormulaCalculator  (in sc/inc/simpleformulacalc.hxx) that you can use to see the result of a formula 
<moggi> [18:42:00] Renato_Ferreira: now the idea is to add an handler to the edit that calculates the input in sc/source/ui/condformat/condformatdlgentry.cxx whenever the content of one of the edit boxes changes
<moggi> Renato_Ferreira: I would start with a simple label below the edit box
<moggi> Renato_Ferreira: there is the Edit::SetModifyHdl for registering a handler that listens to changes in the edit
<moggi> Renato_Ferreira: maybe check the range name dialog that does something similar
<moggi> Renato_Ferreira: sc/source/ui/namedlg/namedefdlg.cxx

> The problem is that we allow formulas in this place and a string without ""
> is a column/range label which might break old systems if we just change it.

Interesting =) I wonder if we can add some entries to the combo box more simply:
"equal to value"
"equal to formula"

that would make the entry simpler, the difference more discoverable and reduce user-error ? =)

Of course - we'd still keep the same underlying enum and do a bit of processing / parsing to select the right version in the dialog ?

=)

Created attachment 109934 [details]
Warning idea

Here is a possible implementation of the suggestion of adding a warning when the user enters unquoted text. I used yellow in the background following the behaviour that syntax errors make it red.

One advantage of this method over splitting the categories is that more "ambiguous" cases, such as 'TRUE' or 'true', would still work - and the warning disappears if you type that - without having to select between value or formula. However, I also see that this idea, due to not requiring an additional warning, might be the cleaner way. What do you think? I'd be interested in trying to implement whatever is decided, it has been fun so far.

What do you think?

Created attachment 110355 [details]
Patch for equal to value vs. equal to formula

I have prepared a patch that creates this second behaviour.
Basically it adds a temporary type of condition SC_COND_EQUAL_VALUE, which the user can select as "equal to value" as opposed to formula. Then anything other than numbers or strings is transformed into a string, and the rest of the code uses the regular SC_COND_EQUAL as before. I think this way there is minimal impact on the rest of the code (didn't have to touch the compiler, etc.)

I also have a patch for the warning idea above, but I'm not sure myself which idea is better for usability.

It needs to be submitted via gerrit - no one will commit it for you from bugzilla.

https://wiki.documentfoundation.org/Development/gerrit

Thanks Joel. I am aware of that, but since there seemed to be some uncertainty about the desired behaviour here, I believed it would be better to first discuss it here. And the patch can be used to see how it currently works. 

If this is the design decision to be made, of course I can go ahead and just push to gerrit.

Renato - wow - this is a great patch =) as Joel says gerrit is best.

Then again - that code boggles the mind - those huge switch statements seem unbeleivably horrible =) I'd love to kill those by using a prettier lookup table and a small loop to map to/fro - could you submit that first as a pure re-factor (ie. without the new feature).

The other thing is - that I don't think we want to touch the global calc conditional formatting type enumeration with a pseudo-type that is isolated to the dialog; so - I think we should append that to the enumeration some way perhaps by having a 'LAST' value in the enum, and using / adding to that in our table inside the dialog code.

But - this is exciting progress =) nice work ! looking forward to your gerrit submits, please CC / poke me there ...

(In reply to Renato Ferreira from comment #9)
> Thanks Joel. I am aware of that, but since there seemed to be some
> uncertainty about the desired behaviour here, I believed it would be better
> to first discuss it here. And the patch can be used to see how it currently
> works. 
> 
> If this is the design decision to be made, of course I can go ahead and just
> push to gerrit.

I'd like us to use ScSimpleFormulaCalculator instead of trying to implement a heuristic for formula token detection. That provides access to the ScTokenArray which allows to check how the formula was parsed.

Otherwise just changing the background is not enough. We would also need a label with a warning message. We are also no longer use the lcl prefix for local functions in calc, instead please put them into an anonymous namespace.

And as Michael already mentioned we should not change ScConditionMode for a hack that is local to the UI code.

(In reply to Markus Mohrhard from comment #11)
> (In reply to Renato Ferreira from comment #9)
> > Thanks Joel. I am aware of that, but since there seemed to be some
> > uncertainty about the desired behaviour here, I believed it would be better
> > to first discuss it here. And the patch can be used to see how it currently
> > works. 
> > 
> > If this is the design decision to be made, of course I can go ahead and just
> > push to gerrit.
> 
> I'd like us to use ScSimpleFormulaCalculator instead of trying to implement
> a heuristic for formula token detection. That provides access to the
> ScTokenArray which allows to check how the formula was parsed.

Hm, maybe that instead of the isQuoted thing? I did instantiate a compiler elsewhere when a condition is entered to check for double or string. So I think I see, we could also invoke the interpreter to check whether the stored expression evaluates to a string before unquoting it? It does seem more reliable, although double-checking that the quotes are there indeed before removing them also seems to be a good idea, if I understood this correctly.

Also, ScSimpleFormulaCalculator does seem very clean to use.

> 
> Otherwise just changing the background is not enough. We would also need a
> label with a warning message. We are also no longer use the lcl prefix for
> local functions in calc, instead please put them into an anonymous namespace.

Please note that I did include a label with a warning message in the attached idea. Which also brings another issue to my mind: even if we go ahead and add the "equal to value" implementation, what happens to "begins with", "contains", etc.? They still require quotes in order to work properly (do they *only* work with strings?), and this fact would be even more obscured by the added functionality. A few ideas:

- Use the warning in all the cases
- Add the "equal to value" condition for convenience, use warning for the others
- Auto-correct the "begin with", "contains" etc. to strings if they are not quoted

Not sure what would be better...

> 
> And as Michael already mentioned we should not change ScConditionMode for a
> hack that is local to the UI code.

Looking more closely at it "begins with" and others also take cell references (so general formulas) as input... So it's basically the same issue as the "equal to" case, I guess.

> Looking more closely at it "begins with" and others also take cell
> references (so general formulas) as input... So it's basically the
> same issue as the "equal to" case, I guess.

Hmm - nasty; incidentally - wrt. your re-factor (which is a big improvement) I was expecting a table like this:

struct {
   ScConditionMode eMode;
   int             nNumEditFields;
   ...
} aEntries[] = {
};

that we could use to do both lookups =) really - performance is irrelevant here and a good representation is more important I think.

Markus - is it best to use a heuristic, or have a 'Value' vs. 'Formula/Reference' type radio-button somewhere abouts ? =)

Anyhow - great to see your code go in Renato.

> Markus - is it best to use a heuristic, or have a 'Value' vs.
> 'Formula/Reference' type radio-button somewhere abouts ? =)

The problem with heuristics is that they are often buggy and are forgotten during the next refactoring. I still would prefer to get to the point where we remove the deprecated column/row labels and until then just provide a warning message.

Another advantage of using the formula compiler is that it knows all corner cases, and you can then operate just on tokens.

Laszlo Kis-Adam committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=fd28dea50930797652afbdce6992bea08c56caa0

tdf#42897 Warn the user if string without quote is entered as condition value.

It will be available in 4.5.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds
Affected users are encouraged to test the fix and report feedback.

Markus Mohrhard committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=c276ca28262c4dc690849d89aa246a308dba1b25

let it sound a bit less dramatic, related tdf#42897

It will be available in 4.5.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds
Affected users are encouraged to test the fix and report feedback.

Markus Mohrhard committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=73779a5ba01d513d1eee498b77ef985bd1c9f1fc

show warning also for existing entries, related tdf#42897

It will be available in 4.5.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds
Affected users are encouraged to test the fix and report feedback.

There is one more commimt coming fixing the handling for the conditions with two edit fields but that needs a bit more debugging.