Re: pgAdmin Event Trigger Compatibility - Mailing list pgadmin-hackers

Hi Akshay,

Thanks for your review and for the suggestions.

On Tue, Jul 23, 2013 at 3:00 PM, Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:

Hi Dinesh 

I have reviewed your event trigger patch. Following is my review comments:

• There is some problem in "refresh" logic. When I clicked on newly created event trigger node, it gets collapsed automatically and the parent node gets selected.

Fixed.

• According to the logic you have disabled the "Default Value" text box in "New Trigger Function..." dialog, but when we select the "IN" radio button on "Parameter" tab it gets enabled. 

Fixed by disabling the radio buttons. Since, these parameter options(IN/OUT/IN OUT/VARIADIC) are not required while creating trigger/eventtrigger function.

• There should be a status bar in "New Event Trigger" dialog. With current implementation user will not know about which inputs are needed/remaining to enable the "OK" button.

Fixed.

• In pgFunction.cpp why we are checking the typname as "\"trigger\"" and "trigger" if these the special case then should we need it for event_trigger as well? Below is the code syntax 
  • "else if (typname == wxT("\"trigger\"") || typname == wxT("trigger") || typname == wxT("event_trigger"))"

Fixed. Here is the link, i have found that the format(typname) may return the value with enclosing.

• On "New Event Trigger" dialog help is not working. When we clicked on Help button it will show "The URL you specified does not exist" in web browser.

Yes, true. I believe, it works when the PG 9.3 comes under "current" state. Below is the URL, what it's get generating.

URL: http://www.postgresql.org/docs/current/static/sql-createeventtrigger.html.

If i replace "current" with "9.3" then the above link is working fine.

@Dave: Correct me if i am wrong here.

 Other than the above code looks good to me.

 

On Mon, Jul 22, 2013 at 6:39 PM, Dinesh Kumar <dinesh.kumar@enterprisedb.com> wrote:

Hi Akshay,

Thank you so much for correcting me on this. It's my bad to forgot the naming convention in the code. Now, ::OnOK() is get populating the required SQL as per the changes.

Please find the new patch and let me know your valuable inputs.

Dinesh

-- 
Dinesh Kumar

Software Engineer

Ph: +918087463317

Skype ID: dinesh.kumar432

www.enterprisedb.com

Follow us on Twitter
@EnterpriseDB 

Visit EnterpriseDB for tutorials, webinars, whitepapers and more

On Mon, Jul 22, 2013 at 5:58 PM, Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:

Hi Dinesh

I have just started reviewing your code and found on issue where any changes made on Event Trigger dialog won't be saved. After looking at the code, you haven't override the ::OnOK() function in dlgEventTrigger.cpp. Can you please fix this and resend the new patch. 

On Wed, Jul 10, 2013 at 9:46 PM, Dinesh Kumar <dinesh.kumar@enterprisedb.com> wrote:

Yes, we can change this enable check box as a radio button. But, "REPLICA/ALWAYS" are two enable's properties. Hence, We have implemented this in the proposed way. Kindly share your opinion on this.

So: "Enabled Replica ( ) Enabled Always ( ) Disabled ( )" ?

Yes Dave.

Dinesh

-- 
Dinesh Kumar

Software Engineer

Ph: +918087463317

Skype ID: dinesh.kumar432

www.enterprisedb.com

Follow us on Twitter
@EnterpriseDB 

Visit EnterpriseDB for tutorials, webinars, whitepapers and more

--

Akshay Joshi
Senior Software Engineer 
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
Phone: +91 20-3058-9522
Mobile: +91 976-788-8246

--

Akshay Joshi
Senior Software Engineer 
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
Phone: +91 20-3058-9522
Mobile: +91 976-788-8246