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 < [email protected]> wrote: > Hi Akshay, > > Thanks for your review and for the suggestions. > > On Tue, Jul 23, 2013 at 3:00 PM, Akshay Joshi < > [email protected]> 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 < >> [email protected]> 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 < >>> [email protected]> 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 < >>>> [email protected]> 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*
