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.

Sign up for GitHub

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

Jump to bottom

Feature/Restructuring OnResetTimeout #164

Closed
wants to merge 4 commits into from
Closed

Feature/Restructuring OnResetTimeout #164

wants to merge 4 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Nov 19, 2018

Implements #2422

This PR is ready for review.

Summary

Implementation of ResetTimeout

CLA

@ghost ghost requested review from BSolonenko and AKalinich-Luxoft November 19, 2018 16:10
@ghost ghost force-pushed the feature/Restructuring_OnResetTimeout branch from fe9e0bc to a142fc3 Compare November 19, 2018 16:32
@KhrystynaDubovyk KhrystynaDubovyk changed the title Feature/restructuring on reset timeout Feature/Restructuring OnResetTimeout Nov 19, 2018
@ghost ghost force-pushed the feature/Restructuring_OnResetTimeout branch from a142fc3 to a1532d9 Compare November 19, 2018 16:50
@LitvinenkoIra LitvinenkoIra mentioned this pull request Nov 20, 2018
Closed
3 tasks
BSolonenko
BSolonenko suggested changes Nov 20, 2018
View reviewed changes
app/model/sdl/Abstract/Model.js Outdated
SDL.SDLController.vrInteractionResponse(
SDL.SDLModel.data.resultCode['TIMED_OUT']
);
// SDL.ResetTimeoutPopUp.resetTimeoutRPCs.removeObject('VR.PerformInteraction');
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ValeriiMalkov please remove commented code.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BSolonenko Fixed in commit a8caa0a

app/view/sdl/shared/interactionChoicesView.js Outdated
self.deactivate('TIMED_OUT');
}, this.timeout
SDL.InteractionChoicesView.deactivate('TIMED_OUT');
// SDL.SDLAbstractView.deactivate();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ValeriiMalkov please remove commented code.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BSolonenko Fixed in commit a8caa0a

css/general.css
margin: 10px;
background-color: black;
}
#ResetTimeoutPopUp .contextView .list {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ValeriiMalkov please add white space.

css/general.css
position: relative;
text-shadow: 1px 1px 2px black, 0 0 6px rgba(255, 0, 0, 0.51), 0 13px 10px black;
}
#ResetTimeoutPopUp .contextView .list .component {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ValeriiMalkov please add white space.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BSolonenko Fixed in commit a8caa0a

AKalinich-Luxoft
AKalinich-Luxoft suggested changes Nov 20, 2018
View reviewed changes
app/model/sdl/Abstract/Model.js Outdated
setTimeout(function() {
if (SDL.SDLModel.data.vrActiveRequests.vrPerformInteraction) { // If VR PerformInteraction session is still active
SDL.SDLModel.onPrompt(message.params.timeoutPrompt);
SDL.ResetTimeoutPopUp.timeoutSeconds['VR.PerformInteraction'] =
(message.params.timeout -(message.params.timeout - 2000))/1000;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ValeriiMalkov why do you need -2000? Please move these magic numbers to a named constants

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AKalinich-Luxoft Fixed in commit a8caa0a

app/view/sdl/AlertPopUp.js

},

setTimerTTS: function(time){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ValeriiMalkov add description

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AKalinich-Luxoft Fixed in commit a8caa0a

app/view/sdl/AlertPopUp.js
}, self.ttsTimeout
);
},
/*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ValeriiMalkov add space between functions

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AKalinich-Luxoft Fixed in commit a8caa0a

app/view/sdl/ResetTimeoutPopUp.js Outdated
disableScrollbar: true,
value: true,

/** Items */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ValeriiMalkov remove this comment

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AKalinich-Luxoft Fixed in commit a8caa0a

app/view/sdl/ResetTimeoutPopUp.js
contentBinding: 'parentView.timeoutString'
}),

player: SDL.AudioPlayer.create(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ValeriiMalkov why do you need player in this prompt?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AKalinich-Luxoft Fixed in commit a8caa0a

app/view/sdl/ResetTimeoutPopUp.js Outdated
/**
* resetTimeoutMoreThenOne function for reset timeout if RPC more then one
*/
resetTimeoutMoreThenOne: function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ValeriiMalkov resetMoreThanOneTimeout

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AKalinich-Luxoft Fixed in commit a8caa0a

app/view/sdl/ResetTimeoutPopUp.js Outdated
if(null !== element){
if(element.checked) {
reset();
return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ValeriiMalkov please rework this condition to get something like:

if (...) {
reset()
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AKalinich-Luxoft Fixed in commit a8caa0a

app/view/sdl/ResetTimeoutPopUp.js Outdated
var method = self.resetTimeoutRPCs[0];
element = document.getElementById(method + 'checkBoxSend');

if(null !== element) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ValeriiMalkov please rework this condition for something like:

if (...) {
DeactivatePopUp(method);
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AKalinich-Luxoft Fixed in commit a8caa0a

app/view/sdl/shared/sliderView.js Outdated
@@ -82,34 +82,39 @@ SDL.SliderView = SDL.SDLAbstractView.create(
SDL.SDLModel.data.resultCode.SUCCESS, this.get('sliderRequestId'),
this.get('adjustControl.sliderValue.value')
);
} else {
} else if(timeout === null) {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ValeriiMalkov use negative condition like else if (timeout !== null) { and remove redundant else if

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AKalinich-Luxoft Fixed in commit a8caa0a

ffw/UIRPC.js
SDL.ResetTimeoutPopUp.set('timeoutSeconds',
{'UI.Alert': request.params.duration/1000,
'TTS.Speak': request.params.duration/1000});

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ValeriiMalkov remove extra space

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AKalinich-Luxoft Fixed in commit a8caa0a

@ghost ghost force-pushed the feature/Restructuring_OnResetTimeout branch from a8caa0a to aa209e1 Compare November 20, 2018 13:54
@ghost ghost force-pushed the feature/Restructuring_OnResetTimeout branch from aa209e1 to 0e141d3 Compare November 20, 2018 14:08
@ghost ghost force-pushed the feature/Restructuring_OnResetTimeout branch 5 times, most recently from 2f5002d to 6993304 Compare November 20, 2018 20:19
@AKalinich-Luxoft AKalinich-Luxoft force-pushed the feature/Restructuring_OnResetTimeout branch from 6993304 to f488770 Compare April 9, 2020 23:34
BSolonenko and others added 4 commits June 4, 2020 15:53
Restructuring OnResetTimeout
5c4fdec 
The ability to send OnResetTimeout for RPS with different interfaces has
been added.
Style issues fixes and fix typo
d90579d
Fix onResetTimeout logic
d16958d 
Fix OnResetTimeout logic for next RPC:

- Alert
- Slider
- ScrollableMessage
- Speak
- PerformInteraction

Also answer to review
@AKalinich-Luxoft
Update styles to align with new features
f2b7d7e
@ydementieiev ydementieiev force-pushed the feature/Restructuring_OnResetTimeout branch from f488770 to f2b7d7e Compare June 4, 2020 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yang1070 yang1070 Awaiting requested review from yang1070

@mrapitis mrapitis Awaiting requested review from mrapitis

@robinmk robinmk Awaiting requested review from robinmk

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@AKalinich-Luxoft @BSolonenko @LitvinenkoIra