On Wed, Mar 22, 2017 at 7:23 PM, Atira Odhner <aodh...@pivotal.io> wrote:
> First - let me try to explain the problem with the failure in the >> feature test. >> We do not load all the javascript libraries, when starting the pgAdmin 4 >> (i.e. the loading the browser/index.html). >> But - load them only when first tree-item of certain type is added. >> e.g. We load the javascript module of 'table', 'index', etc only after >> first tree item of database is added in the tree. > > > I've seen this bug opening the tree by hand, so it is not merely a matter > of opening tree items as quickly as a machine. There are two different code > paths for loading the tree state --- the way the initial load happens upon > expand and the call that is made when the user clicks 'Refresh...' in the > context menu. Maybe a good approach to starting to fix this bug would be to > have only one code path for loading tree state. > What do you mean by that? > I don't think this has to do with loading the javascript modules. > It is - we've personally experienced the same in early phase of the development, when the spinner control was not implemented. We have seen that - when we expand the tree node, it does reloads the server-groups under the server-group node, just because of the same reason. > > >> But - as I said earlier, it represents the data for the tree-item (not >> the item itself). >> Hence - it should be 'itemData', and not 'item'. >> > > 'itemData' and 'item' are both fine with me. > > >> We've already used 'd' to represent the data, 'i' for item, 't' for tree >> at all other places in pgAdmin 4. >> So - it is consistent across the application. > > > Consistency is great, but in this case, aiming for consistency is > hindering the legibility of the application code. At some point, we should > go through the whole application and change all the 'i' to 'item', 'd' to > data', etc. Until then, I think it is worthwhile to sacrifice consistency > in exchange for adding meaning and clarity to the code that is being > updated. > To be honest, not in that case. As the current choice of the alphabet tells enough about the meaning of the code. We're deviating from the actual point at the moment, so - let's concentrate on that first. I would change it to itemData. -- Thanks, Ashesh > > Tira & Joao > > On Wed, Mar 22, 2017 at 2:20 AM, Ashesh Vashi < > ashesh.va...@enterprisedb.com> wrote: > >> >> >> -- >> >> Thanks & Regards, >> >> Ashesh Vashi >> EnterpriseDB INDIA: Enterprise PostgreSQL Company >> <http://www.enterprisedb.com> >> >> >> *http://www.linkedin.com/in/asheshvashi* >> <http://www.linkedin.com/in/asheshvashi> >> >> On Tue, Mar 21, 2017 at 9:20 PM, Atira Odhner <aodh...@pivotal.io> wrote: >> >>> Here is a new patchset that instead hides the spinner when the acitree >>> has been initialized. On average, the spinner seems to disappear about 2 >>> seconds sooner, and I haven't seen flakiness with these changes yet. >>> >>> Tira & Joao >>> >>> On Mon, Mar 20, 2017 at 4:17 PM, Atira Odhner <aodh...@pivotal.io> >>> wrote: >>> >>>> Note that this patch makes the problem of the tree not having loaded >>>> worse, because it only waits for js modules to load rather than arbitrarily >>>> waiting 900ms. >>>> >>> The patch was never intended to remove the spinner earlier. >> Idea was: the spinner should be hidden only after all dependent >> javascript modules are loaded. >> >> I agree about using arbitrary wait was never a good option for sure. >> Though - I still see the flaw in my approach. >> If the dependent script has error, it wont get loaded, and can make the >> things worse by not hiding the spinner at all. >> >> I liked the idea behind your first patch about using the tree events to >> hide the spinner. >> Though - I see scope of improvement in it. >> >> On Mon, Mar 20, 2017 at 3:17 PM, Atira Odhner <aodh...@pivotal.io> wrote: >>>> >>>>> Hi Ashesh, >>>>> >>>>> *Regarding your second patch:* >>>>> >>>>> It looks like your second patch addresses module loading. This is an >>>>> improvement over the previous hard timeout, but won’t do anything for the >>>>> tree issues. The module loading code can also be simplified; we’ve >>>>> attached >>>>> a patchset that is tidier, tests the behavior, and speeds up the polling. >>>>> >>>>> Ashesh, can you explain why you are setting the text on the spinner >>>>> after hiding it, or why you are hiding it rather than removing it? >>>>> >>>> First - let me try to explain the problem with the failure in the >> feature test. >> We do not load all the javascript libraries, when starting the pgAdmin 4 >> (i.e. the loading the browser/index.html). >> But - load them only when first tree-item of certain type is added. >> e.g. We load the javascript module of 'table', 'index', etc only after >> first tree item of database is added in the tree. >> >> Now - when we run the feature tests, it expands all the nodes one by one >> very quickly. >> And, that makes the thing worse, as the javascript module for 'table' is >> still not loaded in the browser, definitely - not immediately after the >> first 'database' item is added, it always takes some time to load the >> module, and takes few fraction of seconds/milliseconds. >> >> Now - the intention was show the spinner, when the dependent javascript >> modules gets loaded. >> Hence - we did not remove the spinner, but - instead only hide it. >> >> And, changed the text to 'Loading the dependent resources...', so that - >> whenever we once again show the spinner, it will show that text. >> I am considering using the tree events to show the spinner again using >> the similar approach used in your first patch. >> >> Then - change the feature test to wait for the spinner to go away before >> expanding the table node. >> >>> *Regarding your first patch:* >>>>> >>>>> Descriptive variable names are clearer than single-letter variable >>>>> names. Could you clean up the first patch to use clearer variables, >>>>> perhaps >>>>> add some tests and break it up into methods so that I can more easily >>>>> understand what your change does? >>>>> >>>> Agree. >> But - as I said earlier, it represents the data for the tree-item (not >> the item itself). >> Hence - it should be 'itemData', and not 'item'. >> >> We've already used 'd' to represent the data, 'i' for item, 't' for tree >> at all other places in pgAdmin 4. >> So - it is consistent across the application. >> >> I would add comments to understand the functionality a lot better. >> >> Will share the patch for the same. >> >> >> -- Thanks, Ashesh >> >>> Thank you, >>>>> >>>>> Tira & George >>>>> >>>>> On Mon, Mar 20, 2017 at 8:03 AM, Dave Page <dp...@pgadmin.org> wrote: >>>>> >>>>>> On Mon, Mar 20, 2017 at 10:24 AM, Ashesh Vashi >>>>>> <ashesh.va...@enterprisedb.com> wrote: >>>>>> > On Fri, Mar 17, 2017 at 8:35 PM, Sarah McAlear <smcal...@pivotal.io> >>>>>> wrote: >>>>>> >> >>>>>> >> Hello, >>>>>> >> >>>>>> >> We agree that we should keep an eye on this and the failing >>>>>> feature tests. >>>>>> >> Our current story touches part of this code, but we won't go into >>>>>> changing >>>>>> >> the library for now. >>>>>> >> >>>>>> >> The patch Tira sent fixes a global variable problem that we found >>>>>> while >>>>>> >> looking into the code that generates the Tree, that >>>>>> >> had the potential to load the tree in an infinite loop. >>>>>> >> Can you apply the patch like this, or would you rather us send it >>>>>> in a >>>>>> >> separate patch email? >>>>>> > >>>>>> > Name of the variable should have been itemData, d (as earlier), as >>>>>> it >>>>>> > represents the data for the expanding node item. >>>>>> > And, that is not good enough for resolving the issue. >>>>>> > >>>>>> > I've two approaches to resolve the idea. >>>>>> > 1. Load the nodes (even when the module representing that node is >>>>>> not yet >>>>>> > loaded) >>>>>> > Pros: >>>>>> > - Nodes will be loaded even when the module for the node type is >>>>>> not yet >>>>>> > loaded. >>>>>> > Cons: >>>>>> > - The nodes with modified url (not the default mechanism) may get >>>>>> failed, if >>>>>> > the module is not yet loaded. >>>>>> > (NOTE: We've not no nodes with that changes at the moment. so - >>>>>> we can >>>>>> > safely ignore it.) >>>>>> > - Operations specific to the nodes will not be honoured until >>>>>> modules is not >>>>>> > loaded. >>>>>> > >>>>>> > 2. Wait for the modules to get loaded before allowing any >>>>>> operations on >>>>>> > them. >>>>>> > Pros: >>>>>> > - All operations will be done on a node only after the respective >>>>>> module is >>>>>> > loaded. >>>>>> > Cons: >>>>>> > - It will block any operations on pgAdmin 4, when the dependent >>>>>> modules are >>>>>> > being loaded. It can annoy the user. >>>>>> > >>>>>> > Please find the patches for both the approaches. >>>>>> > >>>>>> > Dave - please take a look at it. >>>>>> >>>>>> I'll leave you and Tira to figure this one out if you don't mind. My >>>>>> plate is kinda full at the moment. >>>>>> >>>>>> I will note though that neither blocking or potential failures are >>>>>> desirable. >>>>>> >>>>>> -- >>>>>> Dave Page >>>>>> Blog: http://pgsnake.blogspot.com >>>>>> Twitter: @pgsnake >>>>>> >>>>>> EnterpriseDB UK: http://www.enterprisedb.com >>>>>> The Enterprise PostgreSQL Company >>>>>> >>>>> >>>>> >>>> >>> >> >
