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 <[email protected] > 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 < > [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* > -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
