Easy hack?

Is your "observation" still applicable for LO 3.6.1 RC1?

Hello friends
I'm working on it!

hugs

Anderson, are you still working on it? Any chance to see a patch arise from your work? I would gladly review it.

In order to limit the confusion between ProposedEasyHack and EasyHack and to make queries much easier we are changing ProposedEasyHack to NeedsDevEval.

Thank you and apologies for the noise

Hey,

I would like to take this bug, but I some questions before I can start:

Is it still valid to have an empty string for the connection?
The dlg now resides dbaccess/source/ui/dlg?

The described behaviour still occurs in current master.

Jens

Terribly sorry it took me so long to respond.

(In reply to comment #6)

> I would like to take this bug, but I some questions before I can start:

> Is it still valid to have an empty string for the connection?

Yes. That's a PostgreSQL feature.

> The dlg now resides dbaccess/source/ui/dlg?

Yes, AFAIK the code handling that dialog is still there. The dialog itself is the .src/.hrc file, and the .cxx files contain the C++ code that implements the actual actions behind the dialog.

> The described behaviour still occurs in current master.

Yes, as can trivially be checked by "New Database" / Connect to an existing / PostgreSQL / Next, notice that the "Next" field is greyed out until one enters something in "Datasource URL".

Adding self to CC if not already on

The dialog has now been migrated to the new ".ui" system. The dialog itself now resides in dbaccess/uiconfig/ui/connectionpage.ui

The code that handles it is still in dbaccess/source/ui/dlg/ConnectionPage{Setup,}.cxx which also uses dbaccess/source/ui/dlg/ConnectionHelper.cxx

The previous link are about this specific tab page. The whole "tabbed dialog" itself seems to be in:

 dbadmin.cxx
 dbwiz.cxx
 dbwizsetup.cxx

In particular this code looks interesting in dbwizsetup.cxx:

IMPL_LINK(ODbTypeWizDialogSetup, ImplModifiedHdl, OGenericAdministrationPage*, _pConnectionPageSetup)
{
    m_bIsConnectable = _pConnectionPageSetup->GetRoadmapStateValue( );
    enableState(PAGE_DBSETUPWIZARD_FINAL, m_bIsConnectable);
    enableState(PAGE_DBSETUPWIZARD_AUTHENTIFICATION, m_bIsConnectable);
    if (getCurrentState() == PAGE_DBSETUPWIZARD_FINAL)
        enableButtons( WZB_FINISH, true);
    else
        enableButtons( WZB_FINISH, m_bIsConnectable);
    enableButtons( WZB_NEXT, m_bIsConnectable  && (getCurrentState() != PAGE_DBSETUPWIZARD_FINAL));
    return sal_True;
}


So we need to look at how m_bIsConnectable is set from GetRoadmapStateValue( ).

GetRoadmapStateValue( ) is only

dminpages.hxx:        bool                GetRoadmapStateValue() const { return m_abEnableRoadmap; }

which is set in

adminpages.hxx:        void                SetRoadmapStateValue( bool _bDoEnable ) { m_abEnableRoadmap = _bDoEnable;


So we need to chase where SetRoadmapStateValue is called.


Here are the IMO interesting bits:

* ConnectionPageSetup.cxx:

    bool OConnectionTabPageSetup::checkTestConnection()
    {
        return !m_pConnectionURL->IsVisible() || !m_pConnectionURL->GetTextNoPrefix().isEmpty();
    }

    IMPL_LINK(OConnectionTabPageSetup, OnEditModified, Edit*, /*_pEdit*/)
    {
        SetRoadmapStateValue(checkTestConnection());
        callModifiedHdl();
        return 0L;
    }


Just remove the emptiness test in checkTestConnection()? Try that.

> * ConnectionPageSetup.cxx:
> 
>     bool OConnectionTabPageSetup::checkTestConnection()
>     {
>         return !m_pConnectionURL->IsVisible() ||
> !m_pConnectionURL->GetTextNoPrefix().isEmpty();
>     }
 
> Just remove the emptiness test in checkTestConnection()? Try that.

I did that and it's the right location, but I encountered two problems with that:

1. There is a side effect, that also other database will now allow empty Datasource URLs (e.g Spreadsheet, Text, etc.).

My solution for this would be a get database type and handle the test for PGSQL different then the rest. But I'm not sure which DATASOURCE_TYPE is used from /core/dbaccess/source/core/misc/dsntypes.hxx, because there is none for PostgreSQL.

2. It only allows an empty value when something is entered and then deleted, not at the time the page is set up. So enabling of the buttons is triggered through an event.

Currently I'm not familiar enough with the event system in LO to think of a good way to solve this.

About dsn stuff, I submitted a patch to gerrit for dsn part:
https://gerrit.libreoffice.org/#/c/19310/

Julien Nabet committed a patch related to this issue.
It has been pushed to "master":

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

Related tdf#45775: PgSQL new DB allow empty Datasource

It will be available in 5.1.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.

Julien Nabet committed a patch related to this issue.
It has been pushed to "libreoffice-5-0":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=6b8479107482b180723d01d2fcd90120663435c5&h=libreoffice-5-0

Related tdf#45775: PgSQL new DB allow empty Datasource

It will be available in 5.0.4.

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.

Can anyone help me how to compare ::dbaccess::DST_POSTGRES to m_eType variable in checkTestConnection() of "ConnectionPageSetup.cxx". m_eType gives error when used in the function's scope.

(In reply to Akshay Deep from comment #16)
> Can anyone help me how to compare ::dbaccess::DST_POSTGRES to m_eType
> variable in checkTestConnection() of "ConnectionPageSetup.cxx". m_eType
> gives error when used in the function's scope.

I just tested it and I can use m_eType there, no error. The error you are seeing probably has some other cause. I'll gladly take a look if you attach the patch here.

(In reply to Lionel Elie Mamane from comment #17)
> (In reply to Akshay Deep from comment #16)
> > Can anyone help me how to compare ::dbaccess::DST_POSTGRES to m_eType
> > variable in checkTestConnection() of "ConnectionPageSetup.cxx". m_eType
> > gives error when used in the function's scope.
> 
> I just tested it and I can use m_eType there, no error. The error you are
> seeing probably has some other cause. I'll gladly take a look if you attach
> the patch here.

@Lionel : Sorry, It was a typo error. I figured it out. 

Can you help me by helping me find code where the dialog is created and the buttons get enabled. I need to somehow perform a check that if the chosen db is PostgreSQl, then the next button should be enabled by default.

I am trying to look in "dbwizsetup.cxx".

(In reply to Akshay Deep from comment #18)

> Can you help me by helping me find code where the dialog is created and the
> buttons get enabled. I need to somehow perform a check that if the chosen db
> is PostgreSQl, then the next button should be enabled by default.

> I am trying to look in "dbwizsetup.cxx".

Is comment 10 helpful?

(In reply to Lionel Elie Mamane from comment #19)
> (In reply to Akshay Deep from comment #18)
> 
> > Can you help me by helping me find code where the dialog is created and the
> > buttons get enabled. I need to somehow perform a check that if the chosen db
> > is PostgreSQl, then the next button should be enabled by default.
> 
> > I am trying to look in "dbwizsetup.cxx".
> 
> Is comment 10 helpful?

Yes, It was really explanatory and I did what was required. Somehow, the NEXT button needs to be activated as soon as the dialog is created. And here I think I need to look into dbwizsetup.cxx from beginning of [comment 10]. I believe somewhere in ODbTypeWizDialogSetup::createPage()function of dbwizsetup.cxx, we can enable next button by default.

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

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

tdf#45775 PgSQL new DB allow empty Datasource

It will be available in 5.2.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.