move ui.ts from background page to popup

Woo!
Anyways, generally, can you add the following to the pull request message:

General high-level overview of what you are trying to accomplish with this
Overview of how the state is now stored/retrieved for the UI
Testing notes

src/chrome/extension/scripts/background.ts, line 91 [r1] (raw file):
All of these lines should be unindented by 2 spaces if you are pulling them out of the function

src/chrome/extension/scripts/background.ts, line 125 [r1] (raw file):
Duplicate lines

src/chrome/extension/scripts/background.ts, line 125 [r1] (raw file):
Is there a reason we no longer want to block these requests?

src/chrome/extension/scripts/chrome_core_connector.spec.ts, line 27 [r1] (raw file):
You can drop the type signature here (implied from the assignment)

src/chrome/extension/scripts/chrome_core_connector.spec.ts, line 83 [r1] (raw file):
Why is this now being watched (we don't seem to ever check whether it has been called so my guess is we are trying to stop code from executing in it.  Its behaviour seems like it should just proxy through to ChromeCoreConnect::send though)

src/chrome/extension/scripts/chrome_core_connector.spec.ts, line 140 [r1] (raw file):
Where does disconnect come from?

src/chrome/extension/scripts/chrome_core_connector.spec.ts, line 145 [r1] (raw file):
Why the behaviour change here?
Regardless, this line should either be re-added or completely removed.

src/chrome/extension/scripts/chrome_core_connector.ts, line 44 [r1] (raw file):
Can you discuss a bit why we no longer need to be able to support multiple handlers for an update?

src/chrome/extension/scripts/chrome_core_connector.ts, line 97 [r1] (raw file):
:) much cleaner

src/chrome/extension/scripts/chrome_core_connector.ts, line 137 [r1] (raw file):
Can you add a comment about why we might expect this to not be set?  Thanks.

src/chrome/extension/scripts/chrome_core_connector.ts, line 196 [r1] (raw file):
I think this would be more clear as var type = String(update);

src/chrome/extension/scripts/chrome_core_connector.ts, line 204 [r1] (raw file):
This log statement feels a bit unnecessary (at the very least, should probably be moved to console.debug)

src/chrome/extension/scripts/dependencies.js, line 12 [r1] (raw file):
It looks like we are still putting all of the ui object into the background page, can you clarify what exactly is being moved out again?

src/generic_core/core.ts, line 121 [r1] (raw file):
Can you fix the style here?  Thanks.

src/generic_core/remote-connection.ts, line 29 [r1] (raw file):
I think a better description of this variable would be activeEndpoint, thoughts?

src/generic_core/remote-connection.ts, line 227 [r1] (raw file):
You can get rid of the cloneDeep here, not actually needed.

src/generic_core/remote-instance.ts, line 145 [r1] (raw file):
I'd be tempted to say that this should actually look at the state of the underlying connection (and would love to actually remove the state variables from this class actually)

src/generic_ui/scripts/constants.ts, line 1 [r1] (raw file):
It feels like these should still be in the UI module in general, any reason for moving them to their own?

src/generic_ui/scripts/constants.ts, line 3 [r1] (raw file):
Don't need to specify the type for any of these

src/generic_ui/scripts/constants.ts, line 4 [r1] (raw file):
So, any way of moving this to a sub-object of icons?  Then you could have something more like Constants.ICONS.SHARING (with different casing).  I think that might look a bit cleaner.

src/generic_ui/scripts/constants.ts, line 12 [r1] (raw file):
Extra lines

src/generic_ui/scripts/ui.spec.ts, line 30 [r1] (raw file):
This is missing the new fields you added, I'm guessing you should be seeing a type error?  Same question in user.spec.ts (reviewable will not let me comment on that file)

src/generic_ui/scripts/ui.ts, line 131 [r1] (raw file):
:heart: dead code removal

src/generic_ui/scripts/ui.ts, line 145 [r1] (raw file):
So, I have not tested this, however, I'm guessing that the following sequence would be a bug.  Can you confirm whether or not that is the case?  Thanks!
Repro

Click Start a one-time connection, see message to paste
Get distracted, close window
Re-open window to get message

Expected
See link to send to friend
Actual
On copy+paste getting page, see a message about a pending link

src/generic_ui/scripts/ui.ts, line 183 [r1] (raw file):
(this comment spans a couple files)
It seems like it would be easier to have ConnectionState defined as an interface with these values, rename RemoteConnection::getCurrentStateForUI to RemoteConnection::getState, and then just store it as a single variable here.

src/generic_ui/scripts/ui.ts, line 185 [r1] (raw file):
This is going to be called every time we close or re-open the popup.  A couple issues form that:

We listen for updates through these methods, but never stop listening (until we reassign the listeners later).  Semantically, it would make a lot more sense for these methods to be unbound when the popup is closed.
Based off the previous point, if we were to unbind these when we closed the popup, I think there are at least a couple places where we would run into issues from us needing to call these methods even when the popup is closed (e.g. notifications).  We should set up a much more specific separation between things we want to be in the context of a background page that are continually updated and what we are fine with only happening when the window is open

src/generic_ui/scripts/ui.ts, line 193 [r1] (raw file):
===

src/generic_ui/scripts/ui.ts, line 194 [r1] (raw file):
We should have an error here if there was any copy+paste state

src/generic_ui/scripts/ui.ts, line 195 [r1] (raw file):
Can you add a blank line after this if block and then move this.view =... to immediately below it?  It seems like that would clean up readability a bit.

src/generic_ui/scripts/ui.ts, line 639 [r1] (raw file):
This whole block is a space too far to the left

src/interfaces/persistent.d.ts, line 63 [r1] (raw file):
Why does this need to be persistent across restarts

src/interfaces/ui.d.ts, line 26 [r1] (raw file):
Feel free to ignore this note, leaving it so there is some record later on.
It feels like it might be worth it to look at these messages overall and figure out exactly what state we need to get from this and try to collapse things a bit more.  It seems fine for the moment, but we might run into problems if many more fields get added.

src/interfaces/ui.d.ts, line 31 [r1] (raw file):
This should be gettingInstanceIds for clarity.

src/chrome/extension/scripts/background.ts, line 91 [r1] (raw file):
Done.

src/chrome/extension/scripts/background.ts, line 125 [r1] (raw file):
Done.

src/chrome/extension/scripts/background.ts, line 125 [r1] (raw file):
merging error

src/chrome/extension/scripts/chrome_core_connector.spec.ts, line 27 [r1] (raw file):
Done.

src/chrome/extension/scripts/chrome_core_connector.spec.ts, line 83 [r1] (raw file):
What difference does it make, it's just one send command.

src/chrome/extension/scripts/chrome_core_connector.spec.ts, line 140 [r1] (raw file):
from line 81, but I had to search for this, it's old code.

src/chrome/extension/scripts/chrome_core_connector.spec.ts, line 145 [r1] (raw file):
Done.

src/chrome/extension/scripts/chrome_core_connector.ts, line 44 [r1] (raw file):
I dont' know why we had it on the first place, code seems to be written assuming that there is one handler for everything, I mean why would you want to have multiple handlers for notification for example?

src/chrome/extension/scripts/chrome_core_connector.ts, line 137 [r1] (raw file):
Done.

src/chrome/extension/scripts/chrome_core_connector.ts, line 204 [r1] (raw file):
This is only called once, and also it's not a new code.

src/chrome/extension/scripts/dependencies.js, line 12 [r1] (raw file):
ui object lives in popup, we might need to reference it from background page though.

src/generic_core/core.ts, line 121 [r1] (raw file):
What is preferred style?

src/generic_core/remote-connection.ts, line 29 [r1] (raw file):
Done.

src/generic_core/remote-connection.ts, line 227 [r1] (raw file):
Done.

src/generic_core/remote-instance.ts, line 145 [r1] (raw file):
I kinda agree, it's more of a state of a connection not a remote instance, but in another cl please.

src/generic_ui/scripts/constants.ts, line 1 [r1] (raw file):
I've been wanting to create a file where we store all the constants, I feel like we see hard coded constants everywhere in our code, being it in its own file makes it easy to include everywhere, I'd be happy if we start a trend moving all these hard coded literals to this file.

src/generic_ui/scripts/constants.ts, line 12 [r1] (raw file):
Done.

src/generic_ui/scripts/ui.spec.ts, line 30 [r1] (raw file):
Yeah that's why test were failing

src/generic_ui/scripts/ui.ts, line 145 [r1] (raw file):
That's true, I have a note to fix it.

src/generic_ui/scripts/ui.ts, line 183 [r1] (raw file):
I agree, but again this is not a new code.

src/generic_ui/scripts/ui.ts, line 185 [r1] (raw file):
That was my initial plan, but that's not very simple, since our code is written under the assumption that ui was running in the background. For example when connection is dropped we want to be notified even if the popup is not open. This works for now. I looked into this in some extent and I might write a follow up pull request but since this works, I wanted to keep this pull request managable size.

src/generic_ui/scripts/ui.ts, line 193 [r1] (raw file):
Done.

src/generic_ui/scripts/ui.ts, line 194 [r1] (raw file):
Done.

src/generic_ui/scripts/ui.ts, line 639 [r1] (raw file):
Done.

src/interfaces/persistent.d.ts, line 63 [r1] (raw file):
Why would you want to see welcome screen every time you start, and also you might click next and then close the popup and open it and it's annoying having to click next again.

src/interfaces/ui.d.ts, line 31 [r1] (raw file):
Done.

Comments from the review on Reviewable.io

Quoting above comment, I think it got missed:

Anyways, generally, can you add the following to the pull request message:

General high-level overview of what you are trying to accomplish with this
Overview of how the state is now stored/retrieved for the UI
Testing notes

Reviewed files:

src/chrome/extension/manifest.json @ r1
src/chrome/extension/scripts/background.ts @ r2
src/chrome/extension/scripts/chrome_core_connector.spec.ts @ r2
src/chrome/extension/scripts/chrome_core_connector.ts @ r2
src/generic_core/core.ts @ r2
src/generic_core/remote-connection.ts @ r2
src/generic_core/user.ts @ r2
src/generic_ui/scripts/constants.ts @ r2
src/generic_ui/scripts/ui.spec.ts @ r2
src/generic_ui/scripts/ui.ts @ r2
src/generic_ui/scripts/user.spec.ts @ r2
src/interfaces/ui.d.ts @ r2

src/chrome/extension/scripts/chrome_core_connector.spec.ts, line 83 [r1] (raw file):
I was assuming that it did not make a difference, that's why I was a bit confused that the line was added.
Given that it is not affecting functionality, I think it would be better to mock out as little as possible for the sake of overall code clarity.

src/chrome/extension/scripts/chrome_core_connector.spec.ts, line 140 [r1] (raw file):
Sorry, bad phrasing on my part.
I understand that the variable is defined earlier in this file, and that we actually set it at line 106 which gets triggered after we connect.
I think what I was attempting to convey here is that it seems very odd we would fail the "disconnection cleans up state and retries connect." test due to a bug in the initialization code.  It would be nice to make it so that this is not dependent upon that code, however, if it has to be, I'd rather that be checked in a previous test.
Anyways, I just noticed that you did not touch this line, feel free to make an issue and add a TODO instead of changing this.

src/chrome/extension/scripts/chrome_core_connector.ts, line 44 [r1] (raw file):
Notifications is one of the places that it would be the hardest to make a compelling argument for why it would be necessary.  For some of the other ones, however, you could imagine wanting to have a separate environment in a tab from in the browser.
Regardless of whether it is actually needed or not, I still think we should have at least some reason for removing functionality from the code base.
By the way, it does not appear to me that any code was previously written with the assumption that there would only be one handler for each event, can you point to where that was done?

src/chrome/extension/scripts/chrome_core_connector.ts, line 137 [r1] (raw file):
I feel annoying asking for this, but...
Any chance of explaining why that would be the case a bit more (the fact that the if statement exists implies that the case could exist which is about what I got from the comment).
My best guess for the understanding is that we set core.ui to be the ui variable created in the active popup window (or the most-recently-closed popup if one has not yet been opened).  If the user has never launched the popup window, we will never have constructed a UserInterface and thus will not have the variable set.
If you could change the comment to include that (or whatever the actual reason is) as well as to explain why we would be connecting to freedom before that, it would be great (also, if you could explain how we know this will still get called after core.ui is created, I'm not sure I understand that)

src/chrome/extension/scripts/chrome_core_connector.ts, line 206 [r1] (raw file):
Technically called once per update
I fully understand it is not new code, however, I don't think that's a good justification for not cleaning things up when touching the same line.  We have gotten a bit better lately at using accurate log levels and I don't think it would pose a major problem to change this one from info to debug considering it is debug-level data.

src/chrome/extension/scripts/dependencies.js, line 12 [r1] (raw file):
This is tying the variable lifetime to the background page as well.  It's just hard to understand the net effect of this change when we are keeping the UserInterface object fully referenceable from the background page.

src/generic_core/core.ts, line 121 [r1] (raw file):
e.g.
var o2 = {
  a: 0,
  b: 1,
  c: 2,
  'strange key': 3
};

src/generic_core/remote-connection.ts, line 29 [r1] (raw file):
You have it plural, it seems like we will only ever have one, can you clarify that.

src/generic_ui/scripts/ui.spec.ts, line 30 [r1] (raw file):
Ah, didn't catch that was happening.  Thanks.

src/generic_ui/scripts/ui.ts, line 145 [r1] (raw file):
Okay, can you make sure to do that before merging this PR to avoid causing the regression?  Thanks.

src/generic_ui/scripts/ui.ts, line 183 [r1] (raw file):
I don't think it was a code-quality issue until the copying was added here.  The whole part of this file is new code, and unfortunately does not look very clean.  I do not think adding that as an interface and simply saving it would cause much extra effort, and I think it would make a big difference in this file being understandable.  At the least, please make an issue under C:Code-Cleanup for this and add a TODO with the link.

src/generic_ui/scripts/ui.ts, line 185 [r1] (raw file):
In that case, that code definitely feels like it belongs within the background context and should not be moved out.
I'd say that definitely falls under the scope of this pull request as the stated purpose is to move functionality out of the background page.  The way this is currently being done, as far as I can tell, is by creating theUserInterface object in the pop-up, and then adding a reference to it to the background page which depends upon its existence.  If we are really serious about having less in the background page, I think it would be good to try to aim for leaving the code that needs to be in the background page explicitly there and moving other code somewhere that the background page cannot reference it.
If we are going to have the UserInterface object need to be directly accessed by code running in the background page, I'm not sure if I understand the advantage of having it created outside of that scope, and constantly re-created when opening the pop-up.  That just feels like it is going to end up introducing more bugs for no gain.

src/generic_ui/scripts/ui.ts, line 639 [r1] (raw file):
Other left
(from for (var i = 0; i < payload.offeringInstances.length; i++) { to the ending } of the next for loop, every line should be indented by 6 spaces; they are currently all indented by 5)

Comments from the review on Reviewable.io

src/chrome/extension/scripts/chrome_core_connector.spec.ts, line 84 [r1] (raw file):
I disagree, it's clear what this statement does, which is I don't actually want to get anything from core, as opposed to if you mock send, reader might wonder where this send is called from.

src/chrome/extension/scripts/chrome_core_connector.spec.ts, line 141 [r1] (raw file):
I disagree though, previous test case tests connect method and this test what happens when app gets disconnected.

src/chrome/extension/scripts/chrome_core_connector.ts, line 44 [r1] (raw file):
I don't know the historical reasons of why it was written this way, but this works, and honestly I don't understand what you're suggesting we do instead,.

src/chrome/extension/scripts/chrome_core_connector.ts, line 139 [r1] (raw file):
They are loaded asyncronously.

src/chrome/extension/scripts/chrome_core_connector.ts, line 206 [r1] (raw file):
I disagree. I'd rather check this code in. I've touched many filed in this pull request. It's usually fine if it's one comment or two, but I feel like we're losing the focus of what this pull request is all about.

src/chrome/extension/scripts/dependencies.js, line 12 [r1] (raw file):
true.

src/generic_core/core.ts, line 121 [r1] (raw file):
Done.

src/generic_core/remote-connection.ts, line 29 [r1] (raw file):
Done.

src/generic_ui/scripts/ui.ts, line 183 [r1] (raw file):
What is code-quality issue and what's not is somewhat subjective actually.

src/generic_ui/scripts/ui.ts, line 185 [r1] (raw file):
The advantage is that we don't have background page in cloud.

src/generic_ui/scripts/ui.ts, line 639 [r1] (raw file):
Done.

Comments from the review on Reviewable.io

Okay, I'm fine ignoring a couple of the things that popped up.  There are a small (about half a dozen) things that I think still need further discussion, let's try to go through them in-person tomorrow?

Reviewed files:

src/generic_core/core.ts @ r3
src/generic_core/remote-connection.ts @ r3
src/generic_ui/scripts/ui.ts @ r3

src/chrome/extension/scripts/chrome_core_connector.spec.ts, line 84 [r1] (raw file):
What this statement implied to me as a reader of the code (albeit, one that was reviewing it) is that you are attempting to check when getInitialState is called (purpose of a spy).
Send is already mocked out a couple lines below to be a noop (unnecessary given the default behaviour of a spy, but that was existing code)
As a reader of the code and not the author, my first reaction was to attempt to determine why you were wanting to check if a method of a class that you were not testing was being called.  Seeing no attempt to check this, my next action was to go read core.ts and determine what negative side effects were happening in getInitialState that would cause a test to now fail, assuming that I would want to comment on that.  Since all it did was proxy to sendCommand, I then went and read that method.  Since all it did was proxy to send on the passed in browserConnector_ property, I went back to the test to see why that was not properly mocked out.  When I saw that it was mocked out, I was really confused, and thus posted this comment on the code review.
That felt a lot more complicated to me than assuming that the underlying class you were testing would have any of its methods that needed to be mocked out mocked out.
Want to talk about this offline tomorrow?

src/chrome/extension/scripts/chrome_core_connector.spec.ts, line 141 [r1] (raw file):
I completely agree with what you just said (minus the part where you claimed to be disagreeing with me)
The part that feels wrong about this is that the test case is not only testing what happens when the app gets disconnected, but also testing that the mock successfully predicted the underlying behaviour of attaching a listener to the disconnect callback of the port.
My original comment was just me trying to understand the test case without looking too much into it.  This is outside the scope of the pull request, feel free to just leave things.

src/chrome/extension/scripts/chrome_core_connector.ts, line 44 [r1] (raw file):
I'm suggesting we leave the code unaltered unless we have a solid understanding of why it was done that way and see a need for the change.
I'm still a bit unclear on what you meant by "code seems to be written assuming that there is one handler for everything", can you explain that a bit further?

src/chrome/extension/scripts/chrome_core_connector.ts, line 139 [r1] (raw file):
Let's talk about this offline, I don't think I'm understanding you at all here.

src/chrome/extension/scripts/chrome_core_connector.ts, line 206 [r1] (raw file):
That's fine for now, feel free to ignore the comment.
Just to explain my logic for why to change things at times like this though: we don't regularly read the entire code base.  When doing a review, if you notice something that looks off, that's a great time to change it.  I generally think it's better to leave it as feedback in the review than to go change it myself in a separate pull request as that absolutely guarantees a merge conflict for you.

src/chrome/extension/scripts/dependencies.js, line 12 [r1] (raw file):
Any chance of considering a better way to handle this then?

src/generic_ui/scripts/ui.ts, line 172 [r1] (raw file):
Which is definitely a good thing, however, you still rely on all the same data being accessible in the same javascript context.  If this was moving some data so it was no longer accessible in that context (a bit more like how Firefox is doing things), that would feel like a bit of a stronger point, however, I don't think this actually makes any effective change for cloud other than variable renaming.

src/generic_ui/scripts/ui.ts, line 185 [r1] (raw file):
I completely agree, however, using array indexes to bypass typescript type-checking to get implicit any types, and having multiple assignment statements to effectively copy a single block of state out of an object and to variables with a prefix in their name seems like a relatively clear code-quality issue to me.  Want to talk about this offline tomorrow?

Comments from the review on Reviewable.io

I'll fix copy paste problem and add copy paste state as well.
Let's discuss the issue how we register listeners offline.
Meanwhile this pull request is up to date, lotta things have changed, so if you wanna take another pass.

Review status: 0 of 32 files reviewed, 3 unresolved discussions, some commit checks failed.

Comments from the review on Reviewable.io

There are some bugs in this regarding the ui.disconnectedWhileProxying variable:

If I'm currently getting access and it's disconnected (e.g. revoked) while the popup is closed, the popup appears but does not show the disconnected screen
If I'm currently getting access and it's disconnected while the popup is open, the disconnected screen appears fine, but if I close the popup (without clicking confirm), then open it again the screen doesn't reappear
Both of these scenarios leave me with a broken proxy configured and no way to remove it.  It seems like the problem is that ui.disconnectedWhileProxying is only set when proxying stops, but is not reset appropriately the next time the popup opens (because its not set in the constructor or the initial state).

We should also check for similar bugs with other ui public member variables that might be used in polymer bindings which had been expected to persist across opening+closing the popup.  Possibly ui.toastMessage has the same type of issue (though it would be less severe since its just a notification and not really actionable).

Review status: 0 of 23 files reviewed, 3 unresolved discussions, some commit checks failed.

Comments from the review on Reviewable.io

first one is already fixed.
When I started this pull request I tried everything that came to my mind,
but this one is a new feature for example.
On Tue, Jun 2, 2015 at 12:55 PM, dborkan<EMAIL_ADDRESS>wrote:

There are some bugs in this regarding the ui.disconnectedWhileProxying
variable:

If I'm currently getting access and it's disconnected (e.g. revoked)
while the popup is closed, the popup appears but does not show the
disconnected screen
If I'm currently getting access and it's disconnected while the popup
is open, the disconnected screen appears fine, but if I close the popup
(without clicking confirm), then open it again the screen doesn't reappear
Both of these scenarios leave me with a broken proxy configured and no way
to remove it. It seems like the problem is that
ui.disconnectedWhileProxying is only set when proxying stops, but is not
reset appropriately the next time the popup opens (because its not set in
the constructor or the initial state).

We should also check for similar bugs with other ui public member
variables that might be used in polymer bindings which had been expected to
persist across opening+closing the popup. Possibly ui.toastMessage has the
same type of issue (though it would be less severe since its just a
notification and not really actionable).
Review status: 0 of 23 files reviewed, 3 unresolved discussions, some
commit checks failed.
Comments from the review on Reviewable.io
https://reviewable.io:443/reviews/uproxy/uproxy/1161
—
Reply to this email directly or view it on GitHub
https://github.com/uProxy/uproxy/pull/1161#issuecomment-108013650.

Review status: 0 of 29 files reviewed, 1 unresolved discussion, some commit checks pending.

src/generic_ui/scripts/ui.ts, line 142 [r1] (raw file):
Done.

Comments from the review on Reviewable.io

I think I addresses everything I had in mind, can anyone take a look?

So, the easiest way to look at this in Reviewable was for me to just compare it to origin/dev, I apologize for any redundancy.
In general, I'm still not such a huge fan of having the updates registered from UI to core and those continuing to exist even with the panel closed, I think that would be better to discuss offline though.

  Can you explain that?  Thanks!

src/chrome/extension/scripts/chrome_core_connector.ts, line 195 [r8] (raw file):
This seems like it should be included within the conditional block below.

src/chrome/extension/scripts/context.ts, line 20 [r8] (raw file):
So, just want to discuss this a bit more.
context.ts was previously just some nice syntactic sugar to give us access to the variables.  This almost feels like it would be something better to do within the Polymer code, can you explain why it needs to be done here?

src/firefox/data/scripts/firefox_browser_api.ts, line 36 [r8] (raw file):
The emptiness of this function when it is supposed to have a very specific function seems to indicate that this would work better as an event listener of some kind or at least with a better function name ("fulfill" has too much of a connotation within our code that some promise will be fulfilled by calling it)

src/generic_core/globals.ts, line 60 [r8] (raw file):
Nit: this feels like we are reimplementing _.merge

src/generic_core/remote-connection.ts, line 252 [r8] (raw file):
This will only get called if the user calls stop directly and should probably live in the code that gets called whenever our SocksToRtc connection is closed.

src/generic_core/remote-connection.ts, line 286 [r8] (raw file):
This feels like it would be better defined with the name getCurrentState (the get part because it is returning the current state and dropping the UI because it does not seem to be important to the caller that the state is for the UI)

src/generic_core/remote-user.ts, line 351 [r8] (raw file):
Just want to have a small discussion here.
The addition of another arrary of instanceIds here seems to suggest we would be better off sending an array of instances in general and having flags within them to denote the status.
Thanks!

src/generic_core/uproxy_core.ts, line 49 [r8] (raw file):
Okay, this is going to be a bit long and more along the lines of a design discussion.  Sorry about that.
So, as far as I can tell, the way you approached this was to override the update function for one time connection connections to intercept the signalling messages before we send them to the UI.  That seems like one way we could do this.  If we want to do it that way though, I think you can get some nice efficiency boosts by not worrying about doing the JSON encoding/decoding/parsing on the server side and instead just pushing it onto an array and sending the array down.  Additionally, this does change the API between the core and UI (instead of being a single message, SIGNALLING_MESSAGE is now an encoded blob), so that may cause issues if one component gets updated and the other is not.  You would probably want to create a new message to handle that.
Alternatively, it might be easier to have remote connections have public properties that are an array of messages they have sent (and you could just use that to get the current state)

src/generic_core/uproxy_core.ts, line 249 [r8] (raw file):
These do not seem like they would be called in the case of an error, are they necessary?

src/generic_core/uproxy_core.ts, line 481 [r8] (raw file):
This (and the following two properties) both seem like they would be easier to handle if they were in the same object as the copyPasteState.  Thoughts on that?

src/generic_ui/scripts/constants.ts, line 2 [r1] (raw file):
Just checking on this

src/generic_ui/scripts/constants.ts, line 3 [r1] (raw file):
Just checking on this

src/generic_ui/scripts/core_connector.ts, line 33 [r8] (raw file):
Can you explain the move?

src/generic_ui/scripts/ui.spec.ts, line 139 [r8] (raw file):
Extra blank linet

src/generic_ui/scripts/ui.ts, line 126 [r8] (raw file):
Yay!

src/generic_ui/scripts/ui.ts, line 191 [r8] (raw file):
This occurs as well in the Chrome context script, why both places?

src/generic_ui/scripts/ui.ts, line 245 [r8] (raw file):
It seems like this should be in the else block for checking the onlineNetwork state (I think that's what you are trying to accomplish at least).  I'm not really sure on this either, but it might also be a bit cleaner to fully set all the state and then check to figure out what we should do with the view.

src/generic_ui/scripts/ui.ts, line 415 [r8] (raw file):
?

src/generic_ui/scripts/ui.ts, line 539 [r8] (raw file):
?

src/generic_ui/scripts/ui.ts, line 794 [r8] (raw file):
Spacing looks off here, can you double-check that?

src/interfaces/social.ts, line 116 [r8] (raw file):
Can you explain the change here?

Comments from the review on Reviewable.io

ptal

Review status: 23 of 29 files reviewed, 22 unresolved discussions, some commit checks failed.

src/chrome/extension/scripts/background.ts, line 27 [r8] (raw file):
ui is created in popup now, and it needs to have access to browser api.

src/chrome/extension/scripts/chrome_browser_api.ts, line 170 [r8] (raw file):
what do you mean?

src/chrome/extension/scripts/chrome_core_connector.ts, line 191 [r8] (raw file):
So we could allow multiple listeners, and key them based on windowId or something, and when window is closed, register onClose listeners and unregister listeners corresponsing to that window ID. but why?

src/chrome/extension/scripts/chrome_core_connector.ts, line 195 [r8] (raw file):
Done.

src/chrome/extension/scripts/context.ts, line 20 [r8] (raw file):
This predates the name context, this used be called dependencies. this should be loaded once, and is similar to background.js file in background context.

src/firefox/data/scripts/firefox_browser_api.ts, line 36 [r8] (raw file):
I think emptiness of this method indicates that we need this in chrome but not in firefox, similar to onceConnected promise or status object in browser connector.

src/generic_core/globals.ts, line 60 [r8] (raw file):
I just tried _.merge in console and null overwrites the value. Although I don't understand why any of these fields would be null, It can be missing(when adding a new field) or be set, and if it's missing then it would be undefined, in that case it _.merge works well. So I don't understand why we have checks against null to begin with. Thoughts?

src/generic_core/remote-connection.ts, line 252 [r8] (raw file):
Done.

src/generic_core/remote-connection.ts, line 286 [r8] (raw file):
Done.

src/generic_core/remote-user.ts, line 351 [r8] (raw file):
I don't necessarily disagree with this, similarly I think it would make sense to have just one array of instance states, instead of offeringInstanceStatesForUi, and allInstanceIds

src/generic_core/uproxy_core.ts, line 49 [r8] (raw file):
We're not overriding, we're setting the sendUpdate_ method on remote connection. The reason why I didn't  keep this array in remote connection class is because remote connection itself doesn't know whether it's copy-paste connection or not.
I partially agree with your point about JSON encoding decoding.
I don't understand what you mean by one component gets updated and the other is not.

src/generic_core/uproxy_core.ts, line 249 [r8] (raw file):
Yeah we don't care about this. removed it.

src/generic_core/uproxy_core.ts, line 481 [r8] (raw file):
copyPasteState had type ConnectionState which doesn't have getting and sharing messages. We could make a new interface like CopyPasteState that extends ConnectionState and has those fields. Thoughts?

src/generic_ui/scripts/constants.ts, line 2 [r1] (raw file):
Done.

src/generic_ui/scripts/constants.ts, line 3 [r1] (raw file):
lets keep it the way it is.

src/generic_ui/scripts/core_connector.ts, line 33 [r8] (raw file):
It has to be persistent. If you close the window and and open it you wont' that disconnected while proxying thing to be displayed, we could move this to core, and get as part of initial state, but I don't see the point. core connector lives in background page so it works fine.

src/generic_ui/scripts/ui.spec.ts, line 139 [r8] (raw file):
Done.

src/generic_ui/scripts/ui.ts, line 191 [r8] (raw file):
This is when you popup is open and you're disconnected from the app and then connected. you need to get the fresh state, the one in context, is for every time you open a popup, context.ts file is loaded and you get the initial state. this is not triggered when you open a popup.

src/generic_ui/scripts/ui.ts, line 245 [r8] (raw file):
well if onlineNetwork is null we have early return, so if you're here that means you're in else block, I don't like huge else blocks so that's why we have early return, if that's what you mean by this.

src/generic_ui/scripts/ui.ts, line 415 [r8] (raw file):
good point, this is not necessary because we call bringUproxyToFront in chrome before this method is called the same with copy-paste url. But this might break firefox.

src/generic_ui/scripts/ui.ts, line 794 [r8] (raw file):
Done.

src/interfaces/social.ts, line 116 [r8] (raw file):
related to changes in uproxy_core.ts

Comments from the review on Reviewable.io

Reviewed 1 of 6 files at r9, 19 of 20 files at r10.
Review status: 27 of 28 files reviewed at latest revision, 14 unresolved discussions, some commit checks failed.

src/chrome/extension/scripts/background.ts, line 27 [r8] (raw file):
(to clarify for me, the constructor of the UI object needs access to the browser api within the UI context)

src/chrome/extension/scripts/chrome_browser_api.ts, line 170 [r8] (raw file):
Sorry, I meant to delete this after I clarified the questions below.

src/chrome/extension/scripts/chrome_core_connector.ts, line 192 [r8] (raw file):
It does seem a bit unnecessary right now, however, the fact that we are leaving the bound functions dangling at the moment when a window is closed also feels very wrong.
To clarify my question here, I just don't understand the reasoning for moving from a more abstract design (you may attach any number of listeners to an event) to a more specific (you may attach exactly one listener to an event, if there is another listener attached, attaching a new listener will silently detach the previous one before attaching the new one)

src/chrome/extension/scripts/context.ts, line 20 [r8] (raw file):
This does not get loaded once though, it gets loaded every single time that a new panel is opened.  Can you clarify that a bit more?

src/firefox/data/scripts/firefox_browser_api.ts, line 36 [r8] (raw file):
With onceConnected, it has a very clear meaning between the different browsers (a promise that will be resolved once the core connector has connected to the core).  Here, fulfillLaunched implies that a launched promise will be fulfilled when this is called, something that is not true across the different browsers.
If we add a method to a generic interface, I think we want to make sure that it works generically, both in concept and in implementation.  Right now, this fails to be generic as a concept.

src/generic_core/globals.ts, line 62 [r8] (raw file):
From the double-equals, I'm guessing that we're actually checking for undefined but with really terrible style (yay Javascript!)
I'd still say we could get some good cleanup here by using _.merge with a really simple customizer (e.g. (a, b) -> { if (_.isNull(b)) { return a; } }).  That gets rid of a bunch of lines of code and also lets us specify the defaults in only one place.

src/generic_core/remote-user.ts, line 351 [r8] (raw file):
Can you clarify what you mean by not necessarily disagreeing (as in, what implications do you consider that having to the pull request/review)

src/generic_core/uproxy_core.ts, line 49 [r8] (raw file):
Can you clarify what part of JSON encoding/decoding you like and would object to changing?
I'm completely fine with not changing it so that these are properties of the remote connection, I was just trying to think of alternate ideas than having to process things here.
As far as one component getting updated, the long version of that sentence would have been "may cause issues if, in the Chrome browser, one of the app or extension gets updated while the other is still at the previous version".  In general, we should not be making major changes like this to the API without providing at least one version of backwards compatibility.

src/generic_core/uproxy_core.ts, line 436 [r8] (raw file):
That sounds like a good idea

src/generic_ui/scripts/core_connector.ts, line 33 [r8] (raw file):
Okay, that explains the technical reasons fro why it needed to be moved somewhere in the background, can you explain why core connector makes sense though?

src/generic_ui/scripts/ui.ts, line 77 [r10] (raw file):
Cleanup needed

src/generic_ui/scripts/ui.ts, line 234 [r8] (raw file):
The core_connect callback should be triggered whenever it connects, not just on reconnects.  If this is not sufficient, I think something else is broken.

src/generic_ui/scripts/ui.ts, line 245 [r8] (raw file):
I completely agree with you on that in principle, however, in this case, this looks incredibly confusing.  and it's nearly impossible for me to parse.
My general parsing:

Copy the copy/paste state
If there is a copy/paste state, set the view to be copy/paste
Add networks
If there are networks, and we are looking at copy/paste, it's an error (this being based on the view and not an actual conflict is the biggest point that adds confusion in my opinion)
Switch the view to roster, and update sharing information

I think you could do something simpler such as:

Copy copy/paste state
Add networks
Based on copy/paste state and networks, decide on the view (possibly log an error if there is a conflict
Update any sharing information

At this point, if you think what you have now is less confusing, feel free to leave it that way and ignore me.

src/generic_ui/scripts/ui.ts, line 415 [r8] (raw file):
Can you explain why it is being called specifically in the Chrome Browser API but not Firefox?  It seems like we should keep the expected behaviour the same between the browser apis.

src/interfaces/social.ts, line 116 [r8] (raw file):
Can you explain what that change is and why we need to use any now?

Comments from the review on Reviewable.io

OK this has been going on for really long time and it takes some amount of active effort every week just to keep up with dev, and I don't want to point fingers who should've done what while I was working on this pull request. so I want to merge it the way it is, unless there are any fundamental issues.

Review status: 21 of 27 files reviewed at latest revision, 5 unresolved discussions, all commit checks successful.

src/chrome/extension/scripts/context.ts, line 20 [r8] (raw file):
that's true, it need to get the whole state every time panel opens.  because now ui object lives in popup page, so once panel closes it's gone. we need to construct everything every time we open a panel.
When I said it's loaded once, I meant the file is loaded once in popup, it sits on top of requirify stack.

src/generic_core/remote-user.ts, line 351 [r8] (raw file):
That means, I  don't mind one way or another.

src/generic_ui/scripts/core_connector.ts, line 33 [r8] (raw file):
where else do you want me to put it? Not that I'm saying I'll move it somewhere else, but this comment makes no sense.

src/generic_ui/scripts/ui.ts, line 400 [r8] (raw file):
I reverted this.

Comments from the review on Reviewable.io

Hey Salome,
I completely agree with you with wanting to get this merged in, and I'm very sorry if you interpreted anything I said as pointing fingers, that was definitely not my intent.
There were a few discussion points below that I wanted to at least get clarification on before merging this in though.  If you could do me a small favour and avoid acking them until they are fully resolved, it would be greatly appreciated; Reviewable makes it a bit difficult to track down discussions once they are marked as resolved.
Thanks!  I hope we can get these changes merged in today.

Reviewed 6 of 6 files at r11.
Review status: all files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.

src/chrome/extension/scripts/chrome_core_connector.ts, line 191 [r8] (raw file):
Can you please comment on this before resolving it?  Thanks!

src/chrome/extension/scripts/context.ts, line 20 [r8] (raw file):
Can you clarify what you mean by requireify stack?  Thanks.
(also, I think this might get resolved lower down in the PR in the once_connected discussion)

src/firefox/data/scripts/firefox_browser_api.ts, line 36 [r8] (raw file):
Can you please comment on this before resolving it?  Thanks!

src/generic_core/globals.ts, line 62 [r8] (raw file):
Before resolving this, please at least add a TODO for figuring out why we have the checks and cleaning them up?  Thanks!

src/generic_core/remote-user.ts, line 351 [r8] (raw file):
Before closing this, can you either add a TODO or change it?  Thanks!

src/generic_core/uproxy_core.ts, line 49 [r8] (raw file):
Can you please comment on this before resolving it?  Thanks!

src/generic_ui/scripts/constants.ts, line 11 [r11] (raw file):
I don't think either of these strings are needed anymore, for what purpose were they re-added?

src/generic_ui/scripts/core_connector.ts, line 33 [r8] (raw file):
Sorry, let me clarify:
I completely understand why you chose to move this to core_connector for technical reasons (you wanted it somewhere persistent)
My problem is, whether or not the user was disconnected while proxying has absolutely nothing to do with the connection between the UI and the core.  If this needs to be stored, it should be somewhere where the purpose is exceedingly clear.

src/generic_ui/scripts/ui.ts, line 191 [r8] (raw file):
Can you please comment on this before resolving it?  Thanks!

src/generic_ui/scripts/ui.ts, line 245 [r8] (raw file):
Can you please comment on this before resolving it?  Thanks!

src/generic_ui/scripts/ui.ts, line 415 [r8] (raw file):
Okay.  It looks like Chrome still has the call for bringing the window to the front there as well, will you please comment with an explanation explaining why it is needed twice (I'm not quite sure I understand)?  Thanks!

src/generic_ui/scripts/ui.ts, line 944 [r11] (raw file):
It now looks like you could get a lot of simplicity here by changing ui::copyPasteState to have the type CopyPasteState and making this a simple assignment, any reason not to do that?

src/interfaces/social.ts, line 116 [r8] (raw file):
Can you please comment on this before resolving it?  Thanks!

Comments from the review on Reviewable.io

A few comments but generally I think this is nearly done.  I too would like this to be merged soon!  Hopefully where there is still disagreement we can just write TODO or CONSIDER and then move on.  We will likely revisit a lot of this logic when we change the UI-background page / do uproxy-for-cloud, so I'm less concerned with getting it perfect, just making sure there are no bugs and we make note of places we might want to change later on.

Review status: all files reviewed at latest revision, 14 unresolved discussions, all commit checks successful.

src/chrome/extension/scripts/chrome_core_connector.ts, line 192 [r8] (raw file):
My understanding here is that each time the popup opens in Chrome and we create a new ui object, it will register onUpdate handlers that now replace the old onUpdate handlers (since the old ui object is no longer used and ought to be cleaned up at this point).  By making onUpdate only take one handler we don't have to handle unregistering right?  If this is correct I'm fine with it, but can we maybe comment above the function that this is what we're doing?

src/chrome/extension/scripts/context.ts, line 20 [r8] (raw file):
I think this makes sense here - we want this to be done in the same place as the ui object is constructed.  Although maybe it would make more sense to be called in the user_interface.UserInterface constructor itself, if we want this to always be done immediately when creating a new ui object?

src/firefox/data/scripts/firefox_browser_api.ts, line 36 [r8] (raw file):
Let's just add a comment here saying this is not needed for Firefox, and maybe comment why (what is it that Chrome does here that Firefox doesn't need to do?).  We will likely revisit this when we refactor the UI-background code, maybe then we will change the common interface between the browser APIs

src/generic_core/globals.ts, line 62 [r8] (raw file):
This function has been kinda buggy and ugly for a while.  I just added https://github.com/uProxy/uproxy/issues/1653 to clean it up, feel free to mention that issue in a comment but otherwise I think we can address this in a separate pull request.

src/generic_core/remote-user.ts, line 362 [r11] (raw file):
This is kinda confusing -  if the instance is sharing we added it's id to the gettingInstanceIds array?  Seems like one of the names here is backwards.  Or maybe can we just update the array name to be more descriptive (are they getting from us or are we getting from them?)

src/generic_core/uproxy_core.ts, line 49 [r8] (raw file):
Is it easy to just rename this to COPY_AND_PASTE_MESSAGE, then in the ui have 2 handlers: the old one (unchanged) for Update.SIGNALLING_MESSAGE and the new one for COPY_AND_PASTE_MESSAGE?  I think that would address the backwards incompatability concerns and also be clearer on both the core and UI sides that this is for copy+paste only

src/generic_ui/scripts/core_connector.ts, line 33 [r8] (raw file):
I think this is fine, to me core.disconnectedWhileProxying reads like "was the core disconnected while proxying", which is what we want here.  Going forward I think we'll probably revisit all of this stuff when we refactor the UI-background for uproxy-for-cloud

src/generic_ui/scripts/ui.ts, line 221 [r8] (raw file):
When you first install the extension and app and have the extension popup open, won't this.updateInitialState get called twice (once as a result of context.ts when index.html displayed, and a 2nd time as a result of the ui+core connecting)?  If that happens will it be a problem?  If not then this is fine but might still be good to add a comment to the code explaining that this could happen.

src/generic_ui/scripts/ui.ts, line 237 [r8] (raw file):
Just discussed with Jonathan, my understanding is that this was all discussed in person with Salome and Lucas and the conclusion is just to add a TODO comment here

src/interfaces/social.ts, line 116 [r8] (raw file):
I don't have much objection to using any here but it is kinda frowned upon (just ask Lucas and the coding style guide..).  If you have to use any let's add a comment here explaining why it's needed.

Comments from the review on Reviewable.io

ptal

Review status: all files reviewed at latest revision, 11 unresolved discussions, all commit checks successful.

src/chrome/extension/scripts/chrome_core_connector.ts, line 192 [r8] (raw file):
Done.

src/chrome/extension/scripts/context.ts, line 20 [r8] (raw file):
Done.

src/firefox/data/scripts/firefox_browser_api.ts, line 36 [r8] (raw file):
Done.

src/generic_core/remote-user.ts, line 362 [r11] (raw file):
the ones that you're getting instances currently.
How do you want me to call it?

src/generic_core/uproxy_core.ts, line 49 [r8] (raw file):
Done.

src/generic_ui/scripts/constants.ts, line 11 [r11] (raw file):
Done.

src/generic_ui/scripts/ui.ts, line 221 [r8] (raw file):
Yeah that works fine.

src/generic_ui/scripts/ui.ts, line 237 [r8] (raw file):
Done.

src/interfaces/social.ts, line 116 [r8] (raw file):
Done.

Comments from the review on Reviewable.io

Review status: 18 of 27 files reviewed at latest revision, 11 unresolved discussions, some commit checks failed.

src/generic_core/remote-user.ts, line 362 [r11] (raw file):
I'm still confused.  They are getting from me or I am getting from them?  I think you should just add it to the variable name.  Right now the code looks like it means if the instance is sharing, then add it's id to the getting instances list, which looks wrong.

src/generic_core/uproxy_core.ts, line 49 [r8] (raw file):
It looks like this is now calling ui.update twice for each message, is that correct?

Comments from the review on Reviewable.io

Review status: 18 of 27 files reviewed at latest revision, 11 unresolved discussions, some commit checks failed.

src/generic_core/remote-user.ts, line 362 [r11] (raw file):
Should it be instancesGettingAccessFrom or instancesSharingAccess?

src/generic_core/uproxy_core.ts, line 49 [r8] (raw file):
yes. in case you're running old extension and new app. should I not?

Comments from the review on Reviewable.io

Review status: 18 of 27 files reviewed at latest revision, 11 unresolved discussions, some commit checks failed.

src/generic_core/remote-user.ts, line 362 [r11] (raw file):
Maybe instancesGivingLocalUserAccess or instancesSharingWithLocal?  It just seems really weird to say if the instance is sharing then add it to the getting list.  It seems like if the instance is sharing it should be added to the sharing list.

src/generic_core/uproxy_core.ts, line 49 [r8] (raw file):
I think what you want to do is:

if the update is a Update.SIGNALLING_MESSAGE type, you should send it to the extension twice (first as a SIGNALLING_MESSAGE, 2nd as a COPYPASTE_MESSAGE)
you should delete the SIGNALLING_MESSAGE handler in ui.ts so we don't process the update twice - this way new extensions will only process the COPYPASTE_MESSAGE and old extensions will only process the SIGNALLING_MESSAGE.
there are other types of messages, like uproxy_core_api.Update.STATE, uproxy_core_api.Update.STOP_GIVING, etc.  For those I think you just want to send them once with their type, then return so you don't send them a 2nd time as Update.COPYPASTE_MESSAGE (also in the case of .STATE, .STOP_GIVING, etc messages, message.type won't be defined)

Comments from the review on Reviewable.io

Review status: 18 of 27 files reviewed at latest revision, 11 unresolved discussions, some commit checks failed.

src/generic_core/uproxy_core.ts, line 49 [r8] (raw file):
on point 2, what happens when we have an old app and a new extension?

Comments from the review on Reviewable.io

Review status: 16 of 27 files reviewed at latest revision, 11 unresolved discussions, some commit checks failed.

src/generic_core/remote-user.ts, line 362 [r11] (raw file):
Done.

Comments from the review on Reviewable.io

Review status: 15 of 27 files reviewed at latest revision, 11 unresolved discussions, all commit checks successful.

src/generic_core/uproxy_core.ts, line 49 [r8] (raw file):
Just discussed this in person.  Turns out there are other much harder to fix compatibility issues if the user gets a new extension but still has the old app.  We discussed this with Greg and we think for now this will be OK (if the user has a new extension, but an old app, and they copy+paste but close and reopen the popup before their proxy is setup, things won't work correctly), and Jonathan is currently working on a more general solution to updating the chrome app+extension in sync.
I do still think we want to be sure we aren't sending 2 updates to the UI for STATE, STOP_GIVING, etc messages but feel less strongly about making sure the SIGNALLING_MESSAGE/COPYPASTE_MESSAGE updates are backwards compatible

Comments from the review on Reviewable.io

Review status: 15 of 27 files reviewed at latest revision, 11 unresolved discussions, all commit checks successful.

src/generic_core/uproxy_core.ts, line 0 [r8] (raw file):
I don't understand this. Or what do you mean signaling_message being backwards compatible?

Comments from the review on Reviewable.io

:+1:

Review status: 15 of 27 files reviewed at latest revision, 11 unresolved discussions, all commit checks successful.

src/generic_core/uproxy_core.ts, line 57 [r8] (raw file):
This looks better now - in a previous commit I think you had removed the return statement for non SIGNALLING_MESSAGE updates.  We just want to make sure that if update is a uproxy_core_api.Update.STATE or something similar, we only send it to the UI once

Comments from the review on Reviewable.io


@inject with jest test issue

Hi I have some test which is run by jest, and it is quite simple:
it('renders each verify response', () => {
    const gridWrapper = mount(<ResponseGrid advStore={aStore}/>);

    expect(gridWrapper.find(Row)).toHaveLength(1);
  });

ResponseGrid is quite simple with just
export interface ResponseGridProps {
  advStore?: ADVStore;
}

@inject(allStores => ({
  advStore: allStores.advStore as ADVStore
}))
class ResponseGrid extends React.Component<ResponseGridProps, {}> {

  constructor(props) {
    super(props);
  }

  public render() {
    const rows = this.props.advStore.data.map((pvr, i: number) => <Row key={i}/>);
   ... rest ommitted...

Now I have issue that render fails becuse this.props.advStore is undefined in given setup.
Now,
if I comment out @inject from component - it works as it is above.
if I take back @inject and in test I wrap my component in <Provider> - it works
I am using mobx-react 4.2.2.
Now, as per docs, it should work as given above without wrapping in <Provider>  by just specifying
property directly.
However given above to me it seems that @inject overwrites my prop with null/undefined if it
is not provided.
Any hint/tip/suggestion is welcome and appreciated.

@rssole is the advStore null in the outermost component under test, or in some deeper component? Prop injection should work, but only for the outermost component. if there is inject somewhere deeper in the tree it won't pick it up.

This seems to be a duplicate of #260. Please reopen if it isn't


[HTTP Option support] Configure HTTP Option in Route controller from config-network ConfigMap

In Knative serving, the Route controller should read the HTTP option from the  config-network ConfigMap, and configure Kingress spec accordingly.

/assign


Retrieving service host name from rancher catalog question

I can't seem to find in the documentation the way to actually retrieve the service host name from a question in the rancher catalog.
for instance
    - variable: DATABASE_URL
      label: Database URL
      required: true
      type: service

returns the service name Infra/DB for instance. but what i want is the actual dns name. or hostname so i can ping it.
Any ideas?
Thanks

@emilebosch have you solved this? I believe if the value is going to be input by the user launching the catalog, you'd reference it in this manner ${database_url}. Have you taken a look at how existing catalog items are doing this, if not here's a link to the current catalog-templates. Let me know if that helps.

@emilebosch I'll close this out, if you have more questions feel free to open a new ticket.


UWC Maastricht

UWC Maastricht - Maastricht location for the United World College.

@divyavanmahajan Pull request merged. Please notice that free educational license for JetBrains tools is for high and higher schools students, i.e. those who are over 13 of age. Thank you!


EfficientVit-B0 ImageNet Weights

Would it be possible to provide the weights of the EfficientVit-B0 model trained on ImageNet.

You can download the weight using the following link:
https://drive.google.com/file/d/1ganFBZmmvCTpgUwiLb8ePD6NBNxRyZDk/view?usp=sharing

Many thanks for sharing the weights!


Production: "Edit" button on ChantDetail page broken

On the chant detail page, the "Edit" button is broken (see http://<IP_ADDRESS>/chant/699580) - clicking on it for this chant takes you to http://<IP_ADDRESS>/edit-volpiano/699578?pk=699580, which gives a DoesNotExist error. This bug is only present on production: on the corresponding chant detail page on staging (http://<IP_ADDRESS>/chant/699580), clicking on the "Edit" button takes you to http://<IP_ADDRESS>/edit-volpiano/699578?pk=699580&folio=004r, which works.
This issue was previously raised in #297, and should have been fixed by PR #311. I'm not sure why this retrogression has taken place.

we hadn't run git pull in a while on http://<IP_ADDRESS> - having done that, it's fixed!


MSDKUI-1768, MSDKUI-1776: Improve ManeuverItemView

This commit implements the tickets:

MSDKUI-1768: Implement multiline support for instruction and address text
MSDKUI-1776: Component should not auto shrink instruction

Codecov Report

Merging #74 into master will increase coverage by 2.71%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master      #74      +/-   ##
==========================================
+ Coverage   86.59%   89.31%   +2.71%
==========================================
  Files          30       58      +28
  Lines        1969     3649    +1680
==========================================
+ Hits         1705     3259    +1554
- Misses        264      390     +126

Flag
Coverage Δ

#ios
89.31% <ø> (+2.71%)
:arrow_up:

Impacted Files
Coverage Δ

MSDKUI/Classes/ManeuverItemView.swift
95.06% <ø> (ø)

MSDKUI_Demo/IconButton.swift

MSDKUI_Demo/OptionsViewController.swift

MSDKUI_Demo/ManeuversOverviewViewController.swift

MSDKUI_Demo/GuidanceDashboardViewController.swift

MSDKUI_Demo/ExtensionArray.swift

MSDKUI_Demo/ExtensionNMAGeocoder.swift

MSDKUI_Demo/ExtensionUITableViewCell.swift

MSDKUI_Demo/ExtensionBundle.swift

MSDKUI_Demo/ExtensionNMAMapMarker.swift

... and 79 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9f137dd...3e797f1. Read the comment docs.


Hide order column for user account

Disables the order column in Templates List View for normal user - Addresses issue: https://github.com/apache/cloudstack-primate/issues/473

@blueorangutan package

@Pearl1594 a Jenkins job has been kicked to build primate packages. I'll keep you posted as I make progress.

Packaging result: :heavy_check_mark:centos :heavy_check_mark:debian :heavy_check_mark:archive.
QA: http://primate-qa.cloudstack.cloud:8080/client/pr/474 (JID-2168)

@blueorangutan package

@Pearl1594 a Jenkins job has been kicked to build primate packages. I'll keep you posted as I make progress.

Packaging result: :heavy_check_mark:centos :heavy_check_mark:debian :heavy_check_mark:archive.
QA: http://primate-qa.cloudstack.cloud:8080/client/pr/474 (JID-2169)

@blueorangutan package

@rhtyd a Jenkins job has been kicked to build primate packages. I'll keep you posted as I make progress.

Packaging result: :heavy_check_mark:centos :heavy_check_mark:debian :heavy_check_mark:archive.
QA: http://primate-qa.cloudstack.cloud:8080/client/pr/474 (JID-2175)

@blueorangutan package

@davidjumani a Jenkins job has been kicked to build primate packages. I'll keep you posted as I make progress.

@blueorangutan package

@rhtyd a Jenkins job has been kicked to build primate packages. I'll keep you posted as I make progress.

@blueorangutan package

@blueorangutan package

@Pearl1594 a Jenkins job has been kicked to build primate packages. I'll keep you posted as I make progress.

Packaging result: :heavy_check_mark:centos :heavy_check_mark:debian :heavy_check_mark:archive.
QA: http://primate-qa.cloudstack.cloud:8080/client/pr/474 (JID-2195)

Yup, after the api is executed, the order gets reset