Re: pgAdmin Event Trigger Compatibility

Thanks Akshay - patch applied, with a minor change to replace
"EventTrigger" with "Event Trigger" in the treeview. Nice work Dinesh.

On Wed, Jul 24, 2013 at 7:48 AM, Akshay Joshi <akshay(dot)joshi(at)enterprisedb(dot)com
> wrote:

> Hi Dinesh
>
> Code looks good to me. Dave I have finished the review for event trigger
> functionality, you can have a look.
>
>
> On Tue, Jul 23, 2013 at 10:07 PM, Dinesh Kumar <
> dinesh(dot)kumar(at)enterprisedb(dot)com> wrote:
>
>> Hi Akshay,
>>
>> Thanks for your review and for the suggestions.
>>
>> On Tue, Jul 23, 2013 at 3:00 PM, Akshay Joshi <
>> akshay(dot)joshi(at)enterprisedb(dot)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<http://stuff.mit.edu/afs/sipb/user/zacheiss/postgresql-7.1.2/src/backend/utils/adt/format_type.c>,
>> 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(dot)kumar(at)enterprisedb(dot)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.co <http://www.enterprisedb.com/>m<http://www.enterprisedb.com/>
>>>> *
>>>> Follow us on Twitter*
>>>> @EnterpriseDB
>>>>
>>>> Visit EnterpriseDB for tutorials, webinars, whitepapers<http://www.enterprisedb.com/resources-community> and
>>>> more <http://www.enterprisedb.com/resources-community>
>>>>
>>>>
>>>> On Mon, Jul 22, 2013 at 5:58 PM, Akshay Joshi <
>>>> akshay(dot)joshi(at)enterprisedb(dot)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(dot)kumar(at)enterprisedb(dot)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.co <http://www.enterprisedb.com/>m<http://www.enterprisedb.com/>
>>>>>> *
>>>>>> Follow us on Twitter*
>>>>>> @EnterpriseDB
>>>>>>
>>>>>> Visit EnterpriseDB for tutorials, webinars, whitepapers<http://www.enterprisedb.com/resources-community> and
>>>>>> more <http://www.enterprisedb.com/resources-community>
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> *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*
>>>
>>
>>
>
>
> --
> *Akshay Joshi
> Senior Software Engineer
> EnterpriseDB Corporation
> The Enterprise PostgreSQL Company
> Phone: +91 20-3058-9522
> Mobile: +91 976-788-8246*
>

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company