Re: [PATCH] Base: fix incorrect field removal in criterion inside query design view

On Thu, 13 Dec 2012 07:53:24 +0100
Lionel Elie Mamane <lionel@mamane.lu> wrote:

> On Wed, Dec 12, 2012 at 10:53:07PM +0100, Olivier Ploton wrote:
> > Problem: you cannot type a field name inside a criterion
> > if the field name is also the column name, even if it is qualified.
>  
> The "[Table_1].[name]" gets deleted. I see. The intention of this is that
> e.g.:
>
>  [Table_1].[name] < 'some value'
>
> gets displayed as
>
>  < 'some value'
>
> Since the [Table_1].[name] is given by the rows above in the view.
>
> There are actually two problems there:
>
> 1) the [Table_1].[name] should be deleted ONLY at the beginning of
>    the expression, not later.
>
> That's what your patch handles.
>
> Small incremental improvement: use rString.isEmpty() instead of
> rString.getLength()==0.

OK, sorry. Hope there is no fine for awkwardness :-)

> Now, I'm uncertain if we should check "rString.isEmpty()" or
> "i == m_aChildren.begin()". Maybe it is equivalent, but I'm not sure
> yet. Since you have looked at this code more than I did at this point,
> do you have an opinion?

IMO it's not equivalent. The iterator does not recurse inside the tree up to the leafs, it merely 
enumerates direct children. If you add:
   if (i == m_aChildren.begin()) rString.append("*");
at the beginning of the loop, several stars may appear in the output.

To avoid testing emptiness of buffer, I thought about adding a bool (or sal_Bool ?):
    void OSQLParseNode::impl_parseNodeToString_throw (bool& atStart, otherargs)
    {
        if (atStart) {atStart = false; bFilter .... }
    }

    void OSQLParseNode::impl_parseNodeToString_throw (otherargs)
    {
        bool atStart = true;
        impl_parseNodeToString_throw (atSart, otherargs)
    }
but I didn't dare coding it.

> 2) The [Table_1].[name] should be deleted ONLY if it refers to the
>    current column.
>
> Currently *only* the column name is checked, not the source table.

IMO, throwing away column name at the very beginning of the statement is a consistent way of 
simplifying. Not high end but useful and far better than nothing. 

> I tried the attached patch, but it does not work, because it refers to
> the original "real" table, and *not* to the *table* *alias* within the
> query.
>
> Forget the reference to schema and/or catalog in my patch, that was
> borne from the confusion between "real table" and "table instance in a
> query"...
>
> But currently we don't have property to refer to the "query table
> name", that is the "table alias" in the query. My first thought was to
> add such a property and populate it.
>
> However,
>
> The rParam.xField ultimately comes from
> OQueryDesignView::getPredicateTreeFromEntry
>
>         if (pWin)
>         {
>             Reference<XNameAccess> xColumns =
>           pWin->GetOriginalColumns();
>             if (xColumns.is() && xColumns->hasByName(pEntry->GetField()))
>                 xColumns->getByName(pEntry->GetField()) >>= _rxColumn;
>         }
>
> pWin is a OQueryTableWindow; getOriginalColumns() is implemented by the
> base class OTableWindow, which has no notion of Table Alias. So it
> seems that the "getOriginalColumns()" could be shared among all
> instances (aliases) of the same table in query.
>
> So my current thought is that we should add a "sTableAlias" member to
> rParam, populate it in the code above, and pass it along down to
> impl_parseNodeToString_throw, where it can use it instead of
> PROPERTY_TABLENAME in my patch.
>
>
> Do you feel like you can look into this?

I'm trying to do it but I'm having a hard time with aliases... Right now I'm still digging in the 
sources... Wait and see.
--
Olivier.