Skip to content
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

Show the Properties on the web interface #1392

Merged
merged 2 commits into from Nov 27, 2014

Conversation

benallard
Copy link
Contributor

This is a first attempt at it.

The whole ugliness is hidden in the properties[0]._raw_data idiom ...

I'd like to get rid of that, and return a list of dict {source: , name: , value:} from the db API. I don't succeed Please assist.

@benallard
Copy link
Contributor Author

@tardyp: Any opinion about changing the property data type ? Any advice on how to start ?

@tardyp
Copy link
Member

tardyp commented Nov 26, 2014

@benallard , you probably want to do that in the data api.
The problems with lists is that it is ordered, while there is not really an order.
data api, can work well with ordered collections, but that does not work too well for properties.

I think this would need some serious thoughts in order to make something supporting efficient live updates.

If a new property is added, you would send a new property event which would be placed in the end of the list, while it might be placed in the middle.

We have a similar problem with sort options of collection

Anyway, I think we should merge this patch as is, and think about it. maybe document that property data api is not set in marble.

👍

@benallard
Copy link
Contributor Author

I tried to do that, but I always end up getting '[null]' in my properties value. I even modified the types.SourcesProperties class, and this didn't brought me any steps further. The unittest wasn't happy because of other check I wasn't able to find ... That's when I decided to push it as-is.

On 26 Nov 2014, at 17:24, Pierre Tardy notifications@github.com wrote:

@benallard , you probably want to do that in the data api.
The problems with lists is that it is ordered, while there is not really an order.
data api, can work well with ordered collections, but that does not work too well for properties.

I think this would need some serious thoughts in order to make something supporting efficient live updates.

If a new property is added, you would send a new property event which would be placed in the end of the list, while it might be placed in the middle.

We have a similar problem with sort options of collection

Anyway, I think we should merge this patch as is, and think about it. maybe document that property data api is not set in marble.


Reply to this email directly or view it on GitHub.

@tardyp
Copy link
Member

tardyp commented Nov 26, 2014

hum. I see.. indeed this is more work, you'll have to change the unit tests spec

@benallard
Copy link
Contributor Author

My trouble was the isCollection ...

We don't have that much properties, that I wouldn't make the distinction between new one, changed, or such: Upon event, just reload all them ... Don't overcomplicate ... And maybe, it's just enough to react on stepDone event (or how it's called.) as property change only happen there anyway !

I'll make a new PR about the new Property data type, it's easier to discuss once we can see it. In the meantime, this one can go in ! (And don't forget #1380 ...)

@tardyp
Copy link
Member

tardyp commented Nov 27, 2014

The thing is that the way the coffee-script is done is very very generic, so we can't just reload it just for properties.
What we can say is that properties are not a collection. it is actually a dictionary indexed by name.

Then when there is a change, we send the whole properties set to the mq in a message.
Actually, the coffee script "update" message handler supports messages where only what changed is sent over the wire, which could be useful in that case.

@benallard
Copy link
Contributor Author

The trouble with 'keyed by name' is that (and that's no news for you I believe) there are no pure dictionnary in javascript. (everything is Object), so it's quite difficult to discern the (real) properties between the Object members. There are ways (of course), but those are workaround ... That's my biggest concern.

Why make it so complicated when we can simply use an array there ?

Beside, that properties[0]._raw_data is very very ugly ... You don't need to read further to understand that this is hiding a design mistake ... We aim at making a usable API. That is nothing like that !

@tardyp
Copy link
Member

tardyp commented Nov 27, 2014

I completely acknowledge the problem of dictionary iteration. It is a problem we are inheriting from restangular. I did the _raw_data as a workaround until we get better support in restangular.
The fact that our data model is a dictionary, it is not a list.
Using a list is another workaround to the restangular problem, which is more work, and does not allow automatic update easily

sa2ajj pushed a commit that referenced this pull request Nov 27, 2014
Show the Properties on the web interface
@sa2ajj sa2ajj merged commit 7a432ba into buildbot:master Nov 27, 2014
@benallard benallard deleted the properties_web branch November 28, 2014 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants