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

WebUI: RPC refactoring #327

Closed
wants to merge 2 commits into from
Closed

WebUI: RPC refactoring #327

wants to merge 2 commits into from

Conversation

pvomacka
Copy link

@pvomacka pvomacka commented Dec 12, 2016

Pavel Vomacka added 2 commits December 12, 2016 12:40
The rpc module is now separated from display layer.

There are two new global topics:
- 'rpc-start' for showing the widget which indicates execution of rpc calls
- 'rpc-end' for hiding the widget which indicates execution of rpc calls.
These two global topics replace the original methods IPA.display_activity_icon() and
IPA.hide_activity_icon().

There is also new property of a command (notify_globally), which allows to turn off the widget
which indicates network activity. Instead of classic activity indicator there can be
called custom function at the beginning and at the end of network activity.

There are also changes in internal communication in rpc.js module. There are four new
events, two for calling on_success and on_error methods and two for calling custom functions
at the beginning and at the end of network activity.

https://fedorahosted.org/freeipa/ticket/6144
After log in into webui there was 'Authenticating' sign even during loading metadata.
Now while data are loading there is 'Loading data' text. This change requires new global
topic 'set-activity' of activity widget. So for now there is possibility to change
every activity string during running phase just by publishing 'set-activity' topic
and setting new text as first parameter.

Part of: https://fedorahosted.org/freeipa/ticket/6144
@pvomacka
Copy link
Author

The last comment from pvoborni:
"patch 84:

Looks good, works fine, it just needed rebase(I could provide that).

Idea, but that doesn't have to be implemented, or sometime in future,
right now it is not useful: What about providing the rpc object in the
event, and having unique id for each rpc call so that we could track all
rpc which are executed.

patch 101:

  1. It's event name but the property name looks like that it contains a text:
    that.change_text = 'change-activity-text';

Should it be rather: that.change_text_event.

Or even, why does it compare previous text? Does it matter? Wouldn't be
better to have 'set-activity' event. And then the handler would call
something new set_text method:

set_text(new_activity)
that.dots = 0
that.text = new_activity
that.make_step()

--
Petr Vobornik"

@pvomacka
Copy link
Author

Patch 84: Yes, that is really good idea, but as you said - we don't have usecase for it right now. But I created a ticket to not forget about it. https://fedorahosted.org/freeipa/ticket/6553

Patch 101: I changed the name of event to 'set-activity-event', now it accepts one parameter which is new text of activity widget. But it can be extended in the future to accept more parameters and set more attributes.

I think that creating new method "that.set_text" or something similar can lead to calling that method instead of using topics (events). That's the reason why I left the setting of text in anonymous function (event listener).

@pvoborni
Copy link
Member

pvoborni commented Jan 5, 2017

works for me, the travis failure is invalid̈́ - web ui is not related to the tests and pylint passes

@pvoborni pvoborni added the ack Pull Request approved, can be merged label Jan 5, 2017
@pvoborni pvoborni added the pushed Pull Request has already been pushed label Jan 5, 2017
@pvoborni pvoborni closed this Jan 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
2 participants