Hi Joao, I tried the solution/patch you suggested but it is not working for me, I always get *undefined* for preferences values.
Thanks, Murtuza On Thu, Feb 22, 2018 at 3:17 AM, Joao De Almeida Pereira < jdealmeidapere...@pivotal.io> wrote: > Hello Murtuza, > I created a small patch on the way I believe the function should work. Not > 100% sure why we are piggybacking on the window.top, window.opener and so > on to give us information. Nowadays I do not think that loading the JS > files again costs that much in terms of time. > > The issue I came across was that cache_preferences/get_preference behave > in a very strange way. > > Example: > If the cache is populated no problem it returns a value and it is fine > If the cache is not populated it sets a timeout that will check in 1 > second if is the cache is populated if it is not then does nothing, if it > is then returns the value. > The problem is that it returns the value to no place. > > This caching implementation works until this point because for some good > fortune we never call get_preference before we have the cache full. > Is my assessment correct? > > Nevertheless if the caching was using promises we could avoid the problem > above. > > Thanks > Joao > > On Wed, Feb 21, 2018 at 12:50 PM Murtuza Zabuawala < > murtuza.zabuaw...@enterprisedb.com> wrote: > >> Hi Joao, >> >> Thank you for your time in reviewing the patch. >> >> >> On Wed, Feb 21, 2018 at 9:11 PM, Joao De Almeida Pereira < >> jdealmeidapere...@pivotal.io> wrote: >> >>> Hello Murtuza, >>> After reviewing the code I have some suggestions: >>> - We should split the PEP and the features into different patches, or >>> else it becomes very hard to separate Feature code from Code Style change >>> >> I'll keep that in mind next time onwards. >> >>> - The function: `getTextRepresentaionShortcut` has a typo >>> >> I'll fix >> >> that. >> >>> - As a personal point I have a hard time reading multiple declarations >>> of variables in javascript under a single `var`, I prefer 1 `let` per >>> variable >>> >> - I think we should use cameCase for our variables in Javascript code >>> - What is the reason behind the move of the key shortcuts back into the >>> SQLEditorController? This look like something that could be extracted from >>> SQLEditor file into its own >>> >> I'll try to move that code. >> >> >>> - Looks like we are missing some test coverage on the new implemented >>> parts >>> >> I didn't added any new code as such, I just moved out preferences code >> out from main file. >> >>> - The getKeyboardShortcuts function is retrieving information from >>> window.top and window.opener I think that we can come up with a better >>> pattern then this. Relying on window information doesn't look very good. >>> That was the pattern for JQuery that the new frameworks are trying to avoid >>> because polluting the global scope is almost always a Code Smell. >>> >> What do you suggest on this? >> >> >>> >>> >>> I see there was some refactoring on the backend with the creation of >>> RegisterQueryToolPreferences and it is looking good, hope that we can do >>> this in more places specially in the front-end. >>> >>> Using the patch that you sent I passed it through our CI and everything >>> is Passing. >>> >>> Thanks >>> Joao >>> >>> On Tue, Feb 20, 2018 at 1:13 PM Murtuza Zabuawala < >>> murtuza.zabuaw...@enterprisedb.com> wrote: >>> >>>> Hi, >>>> >>>> PFA patch for adding accessibility features in query tool. >>>> RM#2900 >>>> >>>> Please review. >>>> >>>> -- >>>> Regards, >>>> Murtuza Zabuawala >>>> EnterpriseDB: http://www.enterprisedb.com >>>> The Enterprise PostgreSQL Company >>>> >>>>
