New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

User-defined IDs for qooxdoo objects #9544

Open
cboulanger opened this Issue Jul 2, 2018 · 46 comments

Comments

Projects
None yet
5 participants
@cboulanger
Contributor

cboulanger commented Jul 2, 2018

As stated in #9422, qooxdoo should have user-defined IDs for all of its objects. This is useful for at least two use cases

  • Decoupling application components, where components should not be referred to by object reference, but by a „name“ and which can be retrieved at runtime by this name. This is useful especially for plugins which need to hook into application components
  • UI testing where qooxdoo widgets must be identified in an application by external scripts. See #9422, and this example for creating UI tests for qooxdoo with the community edition of TestCafé

As the uses of this system depend very much on the specific application, the spec should be as minimal as possible.

The basic building blocks of the spec could be:

  1. each qooxdoo object gets a property of type string which holds a unique id which can be the value of a DOM ID (see 2). The name of this property could be „objectId“
  2. if the object is a widget (i.e. has a contentElement property), when the ID property is assigned, the element.id of the object's container's DOM node is se to the value of the object id, which puts certain constraints on the value of (1)

Here’s a suggestion how this could look like as a Mixin (pseudo-code, not a real implementation)

qx.Mixin.define("MObjectId",
{
  properties :  {
    objectId : {
      check : "String",
      apply : "_applyObjectId"
    }
  },
  static : {
    __registry : {},
    __idAttribute : "id",
    getObjectById : function(id){
      return MObjectId.__registry[id];
    }
  },
  members : {
    _applyObjectId : function(value) {
      // insert code here to check that id is unique and throw if not 
      MObjectId.__registry[value] = this;
      if (typeof this.getContentElement === "function" ){
        this.getContentElement().setAttribute(MObjectId.__idattribute, value);
      }
    }
  },
  destruct : … remove object from registry

  defer: .. here the mixin could apply itself to `qx.core.Object`
});

More specific solutions such as proposed in #9422 can be layered on top of this.

Questions:

  • should this (a) be made part of ‚qx.core.Object’ or be implemented as a Mixin that is (b) automatically included or (c) needs to be manually included?
  • (updated) should (a) element.id be used to store the ID or (b) a custom (data-/qx-) attribute? (a) would be the intuitive choice, since that's what this attribute is meant for; accessing the node via CSS Selectors is straightforward. (b) would avoid backward-incompatibility issues (if the element.id is already used by an application), but requires a bit more overhead in adressing the node.

(will add more questions as they arise)

@oetiker

This comment has been minimized.

Member

oetiker commented Jul 2, 2018

the 'problem' with this approach is that there may be code that already uses the 'id' attribute for other purposes? maybe even within qooxdoo ?

@johnspackman

This comment has been minimized.

Member

johnspackman commented Jul 2, 2018

I'm not convinced that this will not be very unwieldy for the user of the mixin - while we want IDs to be unique, and this can be achieved by scoping child IDs within parent IDs like "app/some/path/widget", it's not scalable to manually apply absolute IDs to every widget which needs to be unit tested.

An implementation for scoping of child IDs within parent ID is proposed in #9426 - it's fairly far along in development, and AIUI the only thing we're really waiting for with that PR is confirmation that the objectId/ownerObjectId mechanism is required (as opposed to simply depending on getChildControl/_createChildControlImpl).

@cboulanger

This comment has been minimized.

Contributor

cboulanger commented Jul 2, 2018

@johnspackman As said above, this is meant to be a primitive that more scalable solution build upon. There have been objections to a more opinionated approach like yours (which I personally welcome very much) and we should not develop parallel implementations, but layer them appropriately, I think.

@cboulanger

This comment has been minimized.

Contributor

cboulanger commented Jul 2, 2018

@oetiker Maybe then we should stick with the Mixin approach. For example, the mixin could apply itself to qx.core.Object in its "defer" method (just added this to the pseudo code), so that a (@require(MObjectId) compiler hint would load it (or not if it wasn't mentioned). So it remain be the developer's choice to use the object IDs. If people prefer other attributes, a different mixin could be required which extends this one. Does that make sense?

@derrell

This comment has been minimized.

Member

derrell commented Jul 2, 2018

I love that this is being discussed.

In the proposed registry, we have:

MObjectId.__registry[value] = this;

which brings back the object registry that we eliminated previously. (Formerly, it was keyed by hash code.) The problem with a registry of this type is that we then need programmer-created destructor code to remove the object from the registry. @johnspackman's change many months ago removed that requirement.

Regarding the question of ownerObjectId, I identified my use case in #9422 (comment), which is my key desire for this feature.

@cboulanger

This comment has been minimized.

Contributor

cboulanger commented Jul 2, 2018

@derrell This is indeed a problem. But we will need some sort of object registry, otherwise how would you be able to address an object by an ID? But your point also emphasises that the ID mechanism must be opt-in and not by default. By opting into the feature, the developers take the responsibility to deal with the problem. I am not fully sure how/if destruct methods in mixins work, though.

@johnspackman

This comment has been minimized.

Member

johnspackman commented Jul 2, 2018

Hierarchical owner mechanism get around the problem of a universal lookup

@cboulanger

This comment has been minimized.

Contributor

cboulanger commented Jul 2, 2018

If we use element.id and restrict the ID mechanism to objects with have a DOM representation (i.e. widgets), we could do without a registry, since the DOM already has one. However, this would mean that non widget-objects couldn't be addressed.

@cboulanger

This comment has been minimized.

Contributor

cboulanger commented Jul 2, 2018

@johnspackman That's true. But as far as I understood the discussion, there was no consensus on that being everybody's choice (people insisting that they would prefer other ways of generating the IDs) - that's why I tried to take a step back and use a more minimal approach.

@cboulanger

This comment has been minimized.

Contributor

cboulanger commented Jul 2, 2018

Maybe both approaches can be combined? We need absolute ids mostly for UI testing, where an external software needs to address a specific object on the page. So we could use element.id and document.getElementById(id) for this use case, with no object registry required. In addition we use owner/owned hierarchies to include non-UI-objects. This way, it is still possible to assign arbitrary IDs to visible objects and address them later by this ID, even without explicitly establishing a owner/owned hierarchy. The API would then be the same for both methods.

@johnspackman

This comment has been minimized.

Member

johnspackman commented Jul 2, 2018

You could have both, it's just that speaking personally I don't see how manual IDs is going to work on a practical basis, just because the sheer number of IDs that would have to be manually created and managed, and avoiding collisions.

However - thinking about it, widgets are in the ObjectRegistry, as they still need to have dispose actions attached to them. So adding another registry based on IDs won't introduce any further dependencies

@cboulanger

This comment has been minimized.

Contributor

cboulanger commented Jul 2, 2018

You're right, and that is why we need automatic/scalable solution built on top on that. The problem seems to me that we all have our individual projects in mind which all have very different requirements. For example, for testing small demos, even assigning individual IDs is totally sufficient. For large complex apps, that approach is obviously inadequate. What I am trying to achieve is that we agree on some API that individual solutions can build on. Can you outline the API of your owner/owned mechanism?

@cboulanger

This comment has been minimized.

Contributor

cboulanger commented Jul 2, 2018

What about something like this (very rough, not fully thought through)

let IdManager = qx.core.Id.getInstance()
// add a resolver
idManager.addResolver( obj : ResolverInterface );
// add @johnspackman's resolver
idManager.addResolver( qx.core.OwnerIdResolver);
// add a resolver that stores the ID in the node, this could be in a 'defer' method for automatic inclusion
idManager.addResolver( qx.widget.ElementIdResolver);
// try all attached resolvers to get an object that has this ID
idManager.getObjectById("foo:bar:baz");

I am still thinking about which methods the ResolverInterface should contain. Or is this all nonsense and we just let ID systems co-exist without trying to streamline them?

@cboulanger

This comment has been minimized.

Contributor

cboulanger commented Jul 2, 2018

What is really nice about using Element.id is that it will be displayed in the web tools when hovering over the nodes:
bildschirmfoto 2018-07-02 um 20 45 47

@cboulanger

This comment has been minimized.

Contributor

cboulanger commented Jul 4, 2018

I'd really like to get ahead with this, because eventually I want to write a blog about UI-Testing with qooxdoo/TestCafé and I want to have an ID system that is part of qooxdoo rather than introducing some external solution. Would something minimal like this suit everybody?

We introduce a qx.core.id namespace and a qx.core.id.Manager singleton. The manager singleton (initially) has only two public methods (using TypeScript-ish notation here):

  • addResolver( obj: qx.core.id.IResolver): void
  • getObjectById( id: string): qx.core.Object : Uses all registered resolvers to find the object that is referred to by the id

The qx.core.id.IResolver interface enforces only one method

  • getObjectById( id: string): qx.core.Object

This way, we allow different ID systems to happily coexist. Two of them would be shipped with qooxdoo and can be activated on demand: John's owner/owned system and a simple one that uses Element.id for UI testing (for example, with TestCafé).

@johnspackman, what do you think? The namespace and method names can be changed, of course.

@johnspackman

This comment has been minimized.

Member

johnspackman commented Jul 4, 2018

so as I understand it there would be a qx.core.id.MObjectId which is mixed into qx.core.Object, and that adds an id property, plus the qx.core.id.Manager/IResolver/Resolver classes to navigate? In which case, that sounds good to me (I like the namespace too)

@cboulanger

This comment has been minimized.

Contributor

cboulanger commented Jul 4, 2018

Yes - that could be the plan - the only thing I worry about is that id seems such a generic name that might already be used in qooxdoo applications out there? Which would be ok since 6.0 can contain breaking changes, but we want to keep the pain of migrating as minimal as possible...

@cboulanger

This comment has been minimized.

Contributor

cboulanger commented Jul 4, 2018

The nice thing about this system is that you can add/swap resolvers without having to rewrite your tests...

@johnspackman

This comment has been minimized.

Member

johnspackman commented Jul 4, 2018

How about objectId or resolverId?

@cboulanger

This comment has been minimized.

Contributor

cboulanger commented Jul 4, 2018

Let's go with objectId. I try to prepare a PR in the next couple of days.

@oetiker

This comment has been minimized.

Member

oetiker commented Jul 4, 2018

also, I wonder if it is wise to use the 'id' attribute ... would this be configurable? I would be more comfortable with 'data-qx-test-id' or something like this as

document.querySelector("div[data-qx-test-id='something']")

as I assume that some applications may have used the id attribute for their own purposes and hardwiring the id attribute for use in testing would be problematic.

about using the qx.core.id I see no problem since the qx namespace is ours ... this will 'by definition' not cause any clashes

@johnspackman

This comment has been minimized.

Member

johnspackman commented Jul 4, 2018

AIUI the advantage of using the DOM ID attribute is that it shows up in the browser tooltips and is well understood by any 3rd party libraries so IMHO it's a good default, but it could be overridden via a qx.core.Environment setting for those that need it. It's only going to conflict with existing users if they already use DOM ID and they start setting the new objectId property

@oetiker

This comment has been minimized.

Member

oetiker commented Jul 4, 2018

Just making sure that we do not hardwire it ... I am happy with the default being id

@cboulanger

This comment has been minimized.

Contributor

cboulanger commented Jul 4, 2018

The way I would solve that is to have different Resolvers. qx.core.id.ElementIdResolver would use Element.id and could offer optimized methods for using it, so making the attribute configurable wouldn't make sense. There could be an additional qx.core.id.CustomAttributeResolver which could do what you propose. All resolvers would be opt-in so that they do not interfere with existing systems. That's why I am proposing the resolver mechanism in the first place...

@oetiker

This comment has been minimized.

Member

oetiker commented Jul 4, 2018

great ... lets see the code :)

@derrell

This comment has been minimized.

Member

derrell commented Jul 5, 2018

Conceptually this sounds good. I'm interested in seeing what you come up with for code, too!

@level420

This comment has been minimized.

Member

level420 commented Jul 5, 2018

To avoid collisions with existing properties it would maybe be better to not use objectId as a property name, maybe objectRegistryId even if this is a bit lengthy.

@cboulanger

This comment has been minimized.

Contributor

cboulanger commented Jul 5, 2018

First sneak-peak (untested) for those who are interested: https://github.com/cboulanger/qooxdoo/tree/qx.core.id/framework/source/class/qx/core/id

@derrell

This comment has been minimized.

Member

derrell commented Jul 5, 2018

It's good to see some code to be able to think about it in more detail. There are a number of different needs various people have mentioned, for this new module. I'm trying to figure out how this implementation would solve my primary need for these IDs: if code breaks, and I'm looking at the debugger, how do I walk the chain of qooxdoo objects to figure out which specific object, i.e., which line of my code, the problem relates to? It looks here like I'd need access to the object repository, and that the IDs themselves don't tell me how this DOM node is related to some qooxdoo object that I instantiated. I realize that what's done at this point doesn't specifically implement the parent/child resolver, but I'm still not quite seeing it. Does this design allow for what I'm looking for?

@cboulanger

This comment has been minimized.

Contributor

cboulanger commented Jul 5, 2018

No, not yet. This is the implementation of a pluggable ID system in which a resolver that does what you want is inserted. The idea is to have a common interface for resolvers which address the very different use cases that we all have. @johnspackman is working on something in your direction, my question to him would be if he is happy with the current design. My ElementId Resolver is a very simple implementation (which is enough for my use case), others will be more complex...

@cboulanger

This comment has been minimized.

Contributor

cboulanger commented Jul 5, 2018

I was wondering if qx.core.id is really the right namespace. What does the id have to do with core? Especially since there aren't any other sub-namespaces in core. If the id stuff is considered to be useful and given that @johnspackman will contribute another resolver, would qx.id be ok? If we don't want to add a top-level namespace, I'd propose to move it to qx.util instead of core.

@cboulanger

This comment has been minimized.

Contributor

cboulanger commented Jul 5, 2018

Ok, a simple test passes: https://github.com/cboulanger/qooxdoo/blob/qx.core.id/framework/source/class/qx/test/core/ObjectId.js
This demonstrates how the system is activated by @require()-ing the resolver.

@johnspackman

This comment has been minimized.

Member

johnspackman commented Jul 6, 2018

As the ID is a unique ID, and any object which is assigned an ID is to be registered with all Resolver's, why would there ever be more than one Resolver? If there is a conflict, the Manager.getObjectById simply returns the object found by the Resolver which happened to be registered first.

Could this be simplified so that there is a mixin (ie your original code sketch at the top of this issue), and then a single Resolver to implement the registry lookup and getObjectById? Selecting alternative ID mechanism would be done by using an environment key, eg in qx.core.Object:

qx.Class.define("qx.core.Object",
{
  extend : Object,
  include : qx.core.Environment.filter({
    "module.databinding" : qx.data.MBinding,
    "module.logger" : qx.core.MLogging,
    "module.events" : qx.core.MEvent,
    "module.property" : qx.core.MProperty,
    "module.objectid" : qx.core.MObjectId,
    "qx.debug" : qx.core.MAssert,
    "qx.id": qx.core.Environment.select("qx.id.type", {
      "explicit": qx.util.id.MObjectExplicitId,
      "owner": qx.util.id.MObjectOwnerOwnedId
    })
  }),

(I've renamed your original MObjectId to MObjectExplicitId here for illustration purposes)

and in qx.core.id.MObjectExplicitId:

  defer: function(statics) {
    qx.core.id.Manager.setResolver(new qx.core.id.ResolverExplicitId());
  }

in qx.core.id.Manager:

  statics: {
    __resolver: null,
    setResolver: function(resolver) {
      qx.core.Assert.assertNull(this.__resolver);
      this.__resolver = resolver;
    },
    getResolver: function() {
      return this.__resolver;
    },
    getObjectById: function(id) {
        return this.getResolver().getObjectById(id);
    }
  }
});

In this example, the environment selects the correct mixing to use (if at all), and dependency calculation by the compiler/generator causes the correct resolver (ie the one that matches the mixin) to be loaded.

PS I'd like to keep the top level namespace clear if possible - qx.util.id sounds good to me

@cboulanger

This comment has been minimized.

Contributor

cboulanger commented Jul 6, 2018

@johnspackman You need more than one resolver because the way they resolve the ID is not (necessarily) generalizable. If you use the ElementId resolver, the ID is stored in the Element.id attribute ONLY, i.e. it cannot be used to assign IDs which do not have a DOM node. The way this is implemented now leaves the choice of resolver entirely to the developer. If you don't like to store the ID in the ID attribute, you can use a CustomAttribute resolver (not yet there, but trivial to add). If you want to have owner/owned logic, use that resolver, or your own custom resolver, or mix them as you wish. This, it seems to me, is much more flexible, elegant and parsimonious than having to set an environment key. You don't force any one paradigm on developers...

@cboulanger

This comment has been minimized.

Contributor

cboulanger commented Jul 6, 2018

Another argument for using the compiler-hint approach as opposed of setting environment variables could be that plugins or contribs might want to "activate" IDs even though the main application doesn't use them.

@cboulanger

This comment has been minimized.

Contributor

cboulanger commented Jul 6, 2018

Isn't that what we call @require and @include? You can see it in the test code - all you need to do is to @require(qx.core.id.resolver.ElementId), everything else is done by the compiler. If you added @require(qx.core.id.resolver.OwnerId) (or however your class would be called), you'd have your system present PLUS the id would be stored in Element.id.

@johnspackman

This comment has been minimized.

Member

johnspackman commented Jul 6, 2018

yes, sorry - I deleted my comment when I saw the test code and realised what you meant.

@johnspackman

This comment has been minimized.

Member

johnspackman commented Jul 6, 2018

I still don't see the need for multiple resolvers - in fact I think it is more like to be an error if multiple resolvers were loaded because what one resolver considers a unique ID can conflict with another resolver and cause unexpected results in getObjectById. EG, two different compiles could change the load order of the resolver classes, and cause completely different results at runtime)

Loading them via @require is a much better idea than environment though, I like it 👍

@cboulanger

This comment has been minimized.

Contributor

cboulanger commented Jul 6, 2018

I think that is something that can be dealt with on the level of documentation, such as "resolver x doesn't work with resolver y". I think the potential of this system is that you can nicely separate concerns (such as dealing with the DOM node as opposed to how Ids are composed), which you would otherwise have to put into one resolver, producing a lot of redundance. Somebody might want to have OwnerIds but not DOM Ids, some might want to have both ,other would like to combine their custom ID generator with DOM ids, etc.

@cboulanger

This comment has been minimized.

Contributor

cboulanger commented Jul 6, 2018

You are right though that I have to think more thoroughly how the order of registration of resolvers plays out..

@johnspackman

This comment has been minimized.

Member

johnspackman commented Jul 11, 2018

@cboulanger sorry it’s taken so long to look at this - I had a look at your resolver/OwnerId.js, but I don’t think that it helps - the issue as I see it is that these IDs are considered unique in some circumstances but not in others.

The problem starts to appear in the Manager.getObjectById() - as the ID is unique, it is assumed that only one instance can be found for a given ID and that the first Resolver to locate an object by ID must be the correct one. Because Resolvers are activated by compiler hints, the objects are passed to every Resolver so that they can be registered with every Resolver - but this by definition must be redundant unless Resolvers only register some objects, not all. It seems that the design goal is to have a universal registry of objects, which implies that all Resolvers will register all objects.

This is a problem because the definition of an ID can change - what is unique to one Resolver is not unique to another, EG the example where IDs are scoped so a button’s ID might be “apply”, and one Resolver knows how to transform this into “app/path/mydialog/apply” (which is unique) but another resolver just knows it as “apply”

The design gives the impression that Resolvers are interchangeable and an ID is an identifier, but IMHO that’s misleading - changing the Resolver has important side effects such as changing how an “identifier” is interpreted. When you’re writing code that uses identifiers, you need to know if they are unique at the outset (or how to make them unique); imaging writing a contrib, but you do not know whether the application will make the ID unique or whether you are required to make it unique. Worse, a change in how compiler dependencies are calculated will make your application suddenly produce different results.

IMHO the solution is that there are two separate concepts here: (1) how to apply a unique identifier to a piece of the DOM so that some external tool (eg Test Cafe or Selenium etc) can find objects and interface with the application, and (2) how to come up with a unique identifier in the first place.

I think that the ID which is set of each object (ie which is implemented by MObjectId) is required to be unique - and if that means providing a value like ”app/path/mydialog/apply” then that’s what needs to be done. The coder can always do something like myButton.setUniqueId(this.getUniqueId() + “/apply”).

Subsequently, if there is a mechanism created to instantiate objects on demand (whether via the existing getChildControl/_createChildControlImpl and/or via the proposed getObject/_createObject mechanism) these can set the unique ID.

Whether or not there is a Manager.getObjectById is a separate question, but fundamental to this is that the ID assigned to every MObjectId is a fully unique ID.

@cboulanger

This comment has been minimized.

Contributor

cboulanger commented Jul 11, 2018

Thanks, John, for taking the time to think this through. I was also already wondering if we need to separate ID-"Resolvers" from ID-"Generators" but ended up mixing them into one to avoid creating too many namespaces. Would that be something you would find adequate, i.e. if we clearly identify those classes which generate and those which resolve the IDs? I would still think that there is a case to be made for registering generators and resolvers rather than setting just one, if only to support use cases that we cannot yet imagine (especially since we want to support plugins and contribs).

@cboulanger

This comment has been minimized.

Contributor

cboulanger commented Jul 11, 2018

@johnspackman How would you design an API that your getObject/_createObject mechanism could be plugged into?

@johnspackman

This comment has been minimized.

Member

johnspackman commented Jul 12, 2018

I'd design it like #9426 :)

IMHO the issue we're struggling with is how to have a unique ID and a registry without side effects, and although #9426 deals with this it's been on hold.

What stalled #9426 was answering questions about whether creating a parallel getObject/_createObject (compared to the existing getChildControl/_createChildControlImpl) was a good idea.

Last night and this morning, I've been running some tests on my code, having switched my app from using getObject/_createObject to using getChildControl/_createChildControlImpl and whatever the problem was before (ie years ago, problems arose with the appearance queue) it doesn't seem to still be there any more.

So the only question left in that PR is whether it is a good idea to mix together the concepts of "child controls" (which are exclusively about the minutia of a widget's appearance) and application-level objects (eg dialogs, list of customers, arrays of data, etc - which may not even be widgets).

I think that if we can get that PR integrated and with everyone's approval this would solve your, mine, and @derrell's requirements with a really simple API.

I must get something delivered to a customer by the end of this week without fail, and I'd like to get my thoughts together before updating that PR until I get this customer work done, so apologies for asking for yet more time but if you can wait another few days until my deadline is passed that would be great and I'll dedicate some time on Monday to updating it

@cboulanger

This comment has been minimized.

Contributor

cboulanger commented Jul 13, 2018

Sure! I am happy to withdraw my PR if I get some kind of "uniqueId" that can be set on widgets and resolved globally...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment