hidden_files = true not working?

In my init.lua I have:
cmd([[packadd packer.nvim]])

require("packer").startup(function()
  use("wbthomason/packer.nvim")

  use({
    "nvim-telescope/telescope.nvim",
    requires = { { "nvim-lua/plenary.nvim" } },
  })

  use({
    "ahmedkhalf/project.nvim",
    config = function()
      require("project_nvim").setup({
        detection_methods = { ">code" },
      })
    end,
  })
end)

require("telescope").load_extension("projects")

local actions = require("telescope.actions")

require("telescope").setup({
  defaults = {
    mappings = {
      i = {
        ["<C-j>"] = actions.move_selection_next,
        ["<C-k>"] = actions.move_selection_previous,
        ["<esc>"] = actions.close,
      },
    },
  },
  extensions = {
    project = {
      hidden_files = true,
    },
  },
})

But when I switch to my dotfiles project, the files in that project that start with a dot are not visible.
Those files are definitely not git ignored.
Any thoughts about why?
Thanks for a cool tool.
I'm using:

Mac OS X 12.1
alacritty 0.9.0
tmux 3.2a
neovim 0.6.0 with no warnings in checkhealth
latest plugin versions synced via Packer
a minimal set of plugins not shown for formatting, LSP, etc

I can confirm the same issue on Arch Linux, package built via AUR.
The configuration is quite simple in my case:
lua << EOF
require('telescope').load_extension('project')
require('telescope').setup {
  extensions = {
    project = {
      hidden_files = true
    },
  }
}
EOF

noremap \w :lua require('telescope').extensions.project.project{}<CR>

When I type \w and start searching for a file in e.g. .config in my dotfiles, however, nothing shows up.

BTW, above snippet shows me using ahmedkhalf/project.nvim but I have having this issue with nvim-telescope/telescope-project.nvim

I can also confirm the same issue. I came to the issues tab only to file this issue, but it seems that people are already noticing it.

@snoblenet are you still facing the issue?

@dhananjaylatkar yes

@snoblenet can you try following things?

Are you using rg or fdfind? If not, try installing rg.
Confirm that telescope is able to find hidden files using :Telescope find_files hidden=true in your dotfiles.
Try updating nvim to latest stable (v0.7).
If it doesn't work then try with minimal config.

-- Install packer
local execute = vim.api.nvim_command

local install_path = vim.fn.stdpath("data") .. "/site/pack/packer/start/packer.nvim"

if vim.fn.empty(vim.fn.glob(install_path)) > 0 then
    execute("!git clone https://github.com/wbthomason/packer.nvim " .. install_path)
end

vim.api.nvim_exec(
    [[
        augroup Packer
                autocmd!
                autocmd BufWritePost packer.lua PackerCompile
        augroup end
]],
    false
)

local use = require("packer").use

require("packer").startup(
    function()
        -- Package manager
        use("wbthomason/packer.nvim")

        -- Telescope
        use(
            {
                "nvim-telescope/telescope.nvim",
                cmd = "Telescope",
                config = function()
                    require("telescope").setup()
                end,
                requires = {
                    {"nvim-lua/popup.nvim"}
                }
            }
        )

        -- Project manager
        use(
            {
                "nvim-telescope/telescope-project.nvim",
                after = "telescope.nvim",
                config = function()
                    require("telescope").setup(
                        {
                            extensions = {
                                project = {
                                    base_dirs = {"~/code", max_depth = 3},
                                    hidden_files = true
                                }
                            }
                        }
                    )
                    require("telescope").load_extension("project")
                end,
                requires = {"nvim-telescope/telescope.nvim"}
            }
        )
    end
)

Thanks.
I have both rg and fd already installed.
I do not have fdfind.
:Telescope find_files hidden=true works correctly.
I could not get the minimal init.lua working sorry.

You should load extension after you do telescope init.
In your config move require("telescope").load_extension("projects") after require("telescope").setup(...)
...
require("telescope").setup({
  defaults = {
    mappings = {
      i = {
        ["<C-j>"] = actions.move_selection_next,
        ["<C-k>"] = actions.move_selection_previous,
        ["<esc>"] = actions.close,
      },
    },
  },
  extensions = {
    project = {
      hidden_files = true,
    },
  },
})
require("telescope").load_extension("projects")

That fixed it, thanks.


[discussion] Too much tools, features; complexity

Note: I didn't follow the template because this is not really a request or a report, but a discussion
I've found this kit awesome, very well coded, but it's too much complex, much more than a project generated using the Angular CLI for example. I know that the Universal come with other requirements, but this, for example, works fine, and it's much more easy to integrate with an existing project or start developing.
Some points for discussing:

Why Gulp, couldn't use just NPM as the build tool?
Why JSHint - it's because we have JS files, I guess - but, if the Angular app itself is entirely in Typescript, why the Node scripts couldn't be too, using ts-node?
Why PostCSS, Stylelint?
Why having a 'lean Angular' option, if the name of the repository is Universal - couldn't this be in a separate repository?
Why this big demo project using 10 different modules at the app.module, internationalization, and a lot of components? Couldn't it be a simple Hello World, maybe with two simple routes? How I'll know if I remove everything it will still work - what is mandatory and what is not?

Having extra and useful features like PostCSS and Stylelint, for example, is good - if I need it. I know, I could exclude them, but how I will be sure that it would not break something? That's the reason that it should be optional. If I want it, I include it.
And don't get my wrong, it's just my own opinion. I've passed by the source code and I've seen the things that you've done - they're very well coded, and I know that it took a lot of effort to get done.

@rdsedmundo thanks for your kind works, and your thoughts. I believe that this discussion will contribute much to the successful development and maintenance of this project. So, let's start with the points you discussed:

Why Gulp, couldn't use just NPM as the build tool?

I was developing software using Angular much time ago, by using Gulp as a task runner in alignment with Visual Studio 2015. This project inherited much from these previous projects (reused the functionality instead of creating from scratch). Although there're some features that NPM couldn't fully cover as Gulp does, it sounds like a great idea switching to NPM.

Why JSHint - it's because we have JS files, I guess - but, if the Angular app itself is entirely in Typescript, why the Node scripts couldn't be too, using ts-node?

A well-coded app needs to follow very strict guidelines, code styles. The main code style enforcer is obviously TsLint. However, JSHint was really necessary to enforce a consistent code style across the JS parts of this project.
Another brilliant idea is to convert Node scripts into TypeScript and voila: no need to JSHint!

Why PostCSS, Stylelint?

PostCSS - for AutoPrefixer; Stylelint - enforcing code style (I'm aware of those rules need further improvements),

Why having a 'lean Angular' option, if the name of the repository is Universal - couldn't this be in a separate repository?

There's actually a separate repository called ng-seed/spa which has the only lean Angular option. However, the lean Angular on this universal project saves a lot of time during development (reduced build times, HMR, etc).

Why this big demo project using 10 different modules at the app.module, internationalization, and a lot of components? Couldn't it be a simple Hello World, maybe with two simple routes? How I'll know if I remove everything it will still work - what is mandatory and what is not?

The main reason that this project has all those dependencies, is the fact that apart being a seed project for Angular Universal, it also showcases all included modules in alignment with each other.
On the other hand, I could briefly say that almost all dependencies with ngx- prefix can be safely removed from app.module, and app.browser.module. However, this should be documented well so people wouldn't worry about what could be possible side-effects of removing those dependencies.

Please feel free to share your thoughts further, because I believe that these ideas would lead to great contributions to this project and also will make things easier for people who use it. Also, feel free to make contributions/PR's (documentation, functionality, etc).

@theomathieubhvr I'd like to consult you about these topics too, so it would be really cool if you could enumerate here your thoughts on these topics.


Update shebang lines in phamb python scripts

Thanks for making this helpful viral binning tool! I have a minor request to improve the usability of the python scripts in the phamb directory, based on an issue I ran into while running the test data for phamb. See below:
Problem description
Using phamb 1.0.1, installed via conda on a linux server, I got the following error when running the test command:
Command:
phamb/run_RF.py test/contigs.fna.gz test/clusters.tsv test testout

Error:
phamb/run_RF.py: /usr/bin/python: bad interpreter: No such file or directory

Possible causes
I think the issue is caused by the shebang line in phamb/run_RF.py:
#!/usr/bin/python

Because I am running phamb in a conda env, my python is located in ${CONDA_PREFIX}/bin/python. I'm running a very clean system and don't have python installed globally.
Proposed solution
Change the shebang line of phamb/run_RF.py to a more universal shebang (discussed here):
#!/usr/bin/env python

Similarly, phamb/run_RF_modules.py and phamb/split_contigs.py could be changed to use the same shebang (they are currently using #!/bin/python).
I'm happy to make a PR for this minor change, if it helps. Thanks again!

Hi Jackson
Thanks for reporting this and your suggestion. I tested your solution and it resolved the issue, so I will update the Master asap :) Please let me know if it's not working in your env.
Best,
Joachim

Thanks @joacjo !


Clarify blob storage async start_copy_from_url use case

Hi,
I am trying to use the async version start_copy_from_url to copy files from a blob to another. I must admit I am a bit confused by the example
https://github.com/Azure/azure-sdk-for-python/blob/main/sdk/storage/azure-storage-blob/samples/blob_samples_copy_blob_async.py
I would have assumed from the docs
https://azuresdkdocs.blob.core.windows.net/$web/python/azure-storage-blob/12.0.0/azure.storage.blob.aio.html#azure.storage.blob.aio.BlobClient.start_copy_from_url
that the I would get a coroutine to await until the copy has done. But I see that I am supposed to poll and check a status manually. Is that the case? Is there any other way so to avoid explicit polling?
On top of this, is there a documentation about the possible status values?

Thanks for the feedback @andrea-cassioli-maersk . We will investigate & get back to you asap

Hi @andrea-cassioli-maersk Andrea, thanks for your inquiry. For starters, the documentation you linked may be out of date as it seems to be pointing to the package version 12.0.0, whereas the current version at the time of writing this comment is 12.14.1.
With that being said, hopefully this documentation can help clear up any confusion:
start_copy_from_url in BlobClient
In general, the workflow for an asynchronous operation does require you to poll the status by using the get_blob_properties method and inspecting the copy_status value.
For the possible status values, it will either be success if the copy completed synchronously or pending if the copy has been started asynchronously but not yet completed.
Hopefully this clears up any doubts!
Thanks!

Thanks @vincenttran-msft now I understand. Is there any other status? I guess that in case of error an exception will be raised.
On a minor note: you are right I was looking at an older doc version.... just realised it is because in
https://azuresdkdocs.blob.core.windows.net/$web/python/azure-storage-blob/12.14.1/index.html#clients
the link to BlobClient redirects to 12.0.1
https://azuresdkdocs.blob.core.windows.net/$web/python/azure-storage-blob/12.0.0/azure.storage.blob.html#azure.storage.blob.BlobClient

Hi @andrea-cassioli-maersk Andrea, to clarify further on the status, if there is an error during an async copy, no exception would be raised in the client, rather you would also need to check for that when polling. The copy status can have one additional value, failed which indicate something went wrong (there can also be an error on pending status). To see the error message, you can look at the status_description property of the CopyProperties returned by get_blob_properties.
On the docs, I did notice the same behavior with the linking which is pretty strange. I would maybe suggest using our other documentation page as this one seems to be more consistent.


[Task]: Document deep links for FF iota

Task description
There is no place to find the deep links for FF v1. Users have been requesting this information lately.
A deep link for FFv1 looks like this:
iota://wallet/send/iota1qrla2afrc6xy2p52ep20rle4cag42mmpe6wqc49xf3yankp7xfy4w9an36l/?amount=1000&unit=Gi
Requirements
TBD
Creation checklist

[ ] I have assigned this task to the correct people
[X] I have added the most appropriate labels
[X] I have linked the correct milestone and/or project

Closing this as we already have docs for Firefly v2 deeplinks and FF v1 is no longer maintained


[BUG] OpenSearch is mispelled

What is the bug?
OpenSearch is spelled Opensearch in a few places in README and such.
How can one reproduce the bug?
See README.md.
What is the expected behavior?
Fix spelling to OpenSearch.

I believe this could be closed now
cc @prudhvigodithi

[Untraiged]
Closing this issue as the related PR is merged, thanks to @etolbakov. @dblock please re-open if required.
Thank you


fix(gro-454): reduce dom node count in global nav; converts to v3

I've got a bunch of specs to fix before merging but here's the desktop nav. I'm going to follow this up with some clean up and refactoring of the mobile nav as a separate PR.
This gets the number of DOM nodes down from 477 to 188. Our hands are a bit tied with keeping those large sub-menus in the DOM unfortunately.
This will depend on https://github.com/artsy/palette/pull/986 — which should be just about ready to go now.
Will convert off draft once I have specs in.
Updated on left, existing on right:

Our hands are a bit tied with keeping those large sub-menus in the DOM unfortunately.

Does this mean that those parts of the nav can't take advantage of the new Palette work?

Nah, I added a boolean flag to keep things in the DOM — so we use the palette dropdown here for everything but flip that flag for the two sub-menus

OK, this should be in good shape now if you feel like giving it a once over @damassi — I mostly just fixed the specs and some types.


Common RGB struct

Have a common rgb struct
struct RgbColor<RgbVariant, WP, T> {
    r: T,
    g: T,
    b: T,
    _color_space: PhantomData<RGBVariant>,
    _white_point: PhantomData<WP>,
}

then we can define the individual colors as
const Rgb = RgbColor<LinearSrgbSpace, D65, T>
const Srgb = RgbColor<SrgbSpace, D65, T>
const AdobeRgb = RgbColor<AdobeSpace, D65, T>

conversions will be
impl<RgbVariant, WP,T: Float> From<RgbColor<RgbVariant, WP,T>> for Xyz<T> {
    fn from(rgb: Rgb<T>) -> Xyz<T> {
      let conv_matrix = RgbVariant::get_matrix();
      let wp_xyz = WP::get_xyz();

        Xyz {
           .... calculations
        }
    }
}

impl<RgbVariant, WP,T: Float> From<Xyz<T>> for RgbColor<RgbVariant, WP,T> {
    fn from(xyz: Rgb<T>) -> RgbColor<RgbVariant, WP,T> {
      let conv_matrix = RgbVariant::get_matrix();
      let wp_xyz = WP::get_xyz();

        Rgb {
           .... calculations
        }
    }
}

Something like this could be good for linear RGB. I'm not as sure when it comes to gamma encoded RGB, since it may have additional parameters (like the gamma in GammaRgb). Keeping linear and non-linear RGB separate could simplify the RgbColor type a bit:
struct RgbColor<P: Primaries, T: Float> {
    red: T,
    green: T,
    blue: T,
    _primaries: PhantomData<P>,
}

trait Primaries {
    type WhitePoint;

    //Maybe also functions returning the primaries as xyY, etc.
}

...assuming that white point and primaries are connected. I'm not sure if there are any odd stuff going on in that area, so it has to be checked first.

I dont think there is a gamma Rgb. Each Rgb variant has a defautl gamma value which is linear except Srgb which has a non linear gamma.
http://www.brucelindbloom.com/WorkingSpaceInfo.html#Specifications
We can specify these gamma's on the RgbVariants trait.

Simple power law gamma is a thing, but it's not an "official" specification. Just another color compression/non-linearization method and it's sometimes used as a quick-and-dirty sRGB substitute.

Yeah, I misunderstood your comment.

I didn't realize that currently we are assuming all the rgb types are gamma corrected.
We will still need the WP trait on the RgbColor struct. It will be easier for conversions Rgb<D65> -> XYZ<D65> -> Lab<D65> using impls.
Not sure how to keep track of gamma. It is defined for each color type, so we can maybe get it on the rgb variant trait
trait RgbVariant {

    //functions returning the primaries as xyY, etc.
    // function returning gamma for the color space

   // provide default implementation for gamma corrrection
   fn gamma_correction()
}

// override the default gamma conversion for SrgbSpace which does not use a single value for gamma

One issue is that we are not keeping track of whether the color has been corrected for gamma and relying on the end user to do it.

Rgb is currently always assumed to be linear and it's up to the user to find out what the source and destination gamma/encoding should be. Those may even be different, depending on what the program does, and I'm not sure how feasible it is to keep track of the source gamma throughout the pipeline. It doesn't affect any calculations, besides encoding/decoding, and an arbitrary gamma constant will just be dead weight. It's even possible that no encoding or decoding is performed at all.

We will still need the WP trait on the RgbColor struct. It will be easier for conversions Rgb -> XYZ -> Lab using impls.

That would be the type WhitePoint; in the Primaries trait. It's reachable through P as P::WhitePoint.

HSV, HSL, etc. could also be made parametric in the same way as RGB, to reflect what they are based on.

I am more inclined towards separating rgb variant and white point. It is more flexible.
For most users this will not matter as they will use  Srgb defined as either RgbColor<SrgbPrimaries, T> or  RgbColor<SrgbSpace, D65, T>
Consider the case of when Srgb with D50 is needed. If using primaries, this needs to be explicitly defined. However if they are separated, it will be a simple RgbColor<SrgbSpace, D50, T> for the end user. Converison can work through impls on the RgbColor
The way I see conversions working is on the RgbColor itself, rgb points from the color space and white point xy's from the whitepoint trait. Depending on how things work out, we can auto derive conversions on the RgbVariants trait ( via bradford transfrom ) and provide the optimized matrices for the defined color spaces like sRgb-d65 and adobeRgb-d50 etc.
It will be good to have some implementation to see how it shapes up.

Yes, a prototype would be good. Having the same primaries, but a different white point seems strange to me, but I guess it's mathematically possible if they are scaled differently. I'm trying to find something that explains exactly how they are connected (if they are)...

Oee use case I see is that most printers, monitors, camera's etc come with an ICC color profile and these are usually defined with the D50 white point. So Srgb D50 is definitely useful

It's still weird, because the primaries wouldn't look the same. They would intuitively be more blue, or red, or whatever. I'm currently reading section B.2 in this specification to see what they have to say about chromatic adaption.

Example: If D65 is selected as the sRGB adapted white, the chromatic adaptation transform will go from D65 to D50, the resulting D50 values will be encoded in the mediaWhitePoint tag, and the sRGB primaries will be adapted to D50 for encoding in the ICC profile. However, if D50 were selected as the sRGB adapted white, chromatic adaptation would not be necessary, D65 values would be encoded in the mediaWhitePoint tag, and the sRGB primaries would be used in the profile without adaptation.

It's a bit cryptic. At least the last sentence. What I think it's trying to say is that primaries and white point is completely disconnected, but I'm not 100% sure. If that's the case, then we should also disconnect them, as you are suggesting. Your SrgbSpace would basically represent the  ITU-R BT.709  primaries.

After looking at the test data, maybe a good idea to encode the gamma/linear trait in the Rgb. Looking at the options:
Assume linear

Current behaviour
User needs to manually correct for gamma and this depends on the rgb space.
Easy to miss the gamma correction step.
Not clear when gamma correction is needed.

Assume Gamma

Most values shown are gamma corrected.
Every calculation like mix, add etc must convert to linear and from linear.
Not practical.

Add GammaEncoding trait

Srgba becomes RgbColor<Linear, Srgb, D65, T > or RgbColor<Gamma, Srgb, D65, T>
What should be the default?
How should it be made clear to the users that rgb's loaded from images or color list from somewhere are gamma encoded and should be loaded as such.

I have some more questions:

How do we express the gamma exponent?
What happens with the gamma parameter when the color is converted to ex. XYZ? Does it go there, too?

What should be the default?

sRGB seem to be the big one.

How should it be made clear to the users that rgb's loaded from images or color list from somewhere are gamma encoded and should be loaded as such.

I'm not sure we can encode this into the types at all. Nothing will prevent anyone from just using Rgb::new and doing whatever they want. There is no way to detect an encoding and I wouldn't want to restrict the users too much in this area. The best option I know of is to have lots of information and examples where the proper procedure is used.
An additional way to discourage direct use of Rgb could be to rename it to LinearRgb. It's a long name and it's there, in your face, all the time. People would then hopefully think one more time before using it directly.

How  do we express the gamma exponent?

It is  part of the Rgb variant. The gamma values are listed here http://www.brucelindbloom.com/WorkingSpaceInfo.html#Specifications. Only Srgb has a non standard calculation. The rest all are color^gamma and its inverse. So we can enocde it directly in the RgbVariant trait.

What happens with the gamma parameter when the color is converted to ex. XYZ? Does it go there, too?

Gamma is not a parameter as such, it just indicates whether the color is gamma corrected or not. Xyz conversions are only to the Linear Rgb Types.

It is part of the Rgb variant. The gamma values are listed here http://www.brucelindbloom.com/WorkingSpaceInfo.html#Specifications. Only Srgb has a non standard calculation. The rest all are color^gamma and its inverse. So we can enocde it directly in the RgbVariant trait.

Sure, but I meant the GammaRgb case. I'm having a feeling that it will be easier to keep encoded and linear RGB apart for some more time, and not cram everything into one type. I know that I'm usually promoting less duplication, but I think that this case may be fit for an exception. The encoded types are only used as a temporary state, so they will not have any fancy operations or anything. What about this structure:
//Encoded with standardized encodings, such as sRGB
//(maybe with a Whitepoint parameter)
struct EncodedRgb<Encoding, T> {...}

//Choose-your-own-gamma encoded RGB
struct GammaRgb<Primaries, Whitepoint, T> {...}

//Good old linear RGB
struct Rgb<Primaries, Whitepoint, T> {...}

EncodedRgb and GammaRgb could also bundle an alpha component, like they currently do, for the sake of simplicity. An other alternative is to make them wrappers, like Alpha, but that may be too much.

You'll still have to encode gamma as part of the RgbVariant trait for conversions to and from linear. Not sure if separating them buys us anything.
We'll most likely define
type Rgb<T> = RgbColor<Gamma, SrgbSpace, T>;
type LinearRgb<T> = RgbColor<Linear, SrgbSpace, T>;

which will cover most of the use cases. We can discuss this more when implementing it. It will give us a much clearer picture of how the types interact.
Another thing is the add, sub, mul traits and how will they work in the gamma space. I am not 100% clear on this.

Another thing is the add, sub, mul traits and how will they work in the gamma space. I am not 100% clear on this.

They don't, really. That's the thing with this whole library. You won't get good results in gamma space, so it's greatly discouraged. The same goes for mixing, scaling pictures, rotating, whatever. There's still the possibility to force gamma encoded values to be taken as linear, as an escape hatch, but that's it. I would really like to keep the distinction as obvious as possible, but with a small escape hatch for when the user knows what's up.
Linear and encoded RGB will effectively be two different types with the same kind of content, if this same distinction is preserved, so the only duplication will more or less be the structs themselves, the constructors, and nothing else.
Those things, and the custom gamma exponents, are why I'm thinking that it could be less complex to keep them separated.

We can discuss this more when implementing it. It will give us a much clearer picture of how the types interact.

Possibly, but we must think of it as a prototype, and not necessarily the final implementation. No decision has been made, so no strings attached.

Exactly

I'm just making sure we are on the same page :smile:

I think this fits within the scope of 0.3.0, so I'm scheduling it, but we can always reschedule if it turns out to be problematic in any way.

Here is how I am planning to implement this

Existing Rgb will be the gamma encoded value (which seems to be the default in most cases) with the added primaries trait struct Rgb<Primaries, Whitepoint, T> {...}
Add linear rgb struct LinearRgb<Primaries, Whitepoint, T> {...}
Provide converisions between the two.
Define Srgb<T> = Rgb<SrgbSpace, D65, T>>, LinearSrgb<T> = LinearRgb<SrgbSpace, D65, T>> etc

Any thoughts?
What is still unclear is whether the operations like blend, add , multiply etc should be converted into linear before applying them or apply as is for each linear and gamma encoded types.

I'm writing up an additional proposal for the pixel encoding types. It's very similar to what you are planning and will address some of the questions.

I think it's good to take care of what's discussed in #58 in combination with this, since they are so closely connected. It could also be done as a two step process, where the linear RGB struct is changes separately from the encoded variants, in whatever order is best.

What is still unclear is whether the operations like blend, add , multiply etc should be converted into linear before applying them or apply as is for each linear and gamma encoded types.

My proposal is to not implement them for encoded types at all, and thereby prevent any accidental mixups between what's linear and what's not.


didRegisterForPushNotification callback

Not sure if here is the best place for raising this but the documentation for push notifications doesn’t mention anything about the didRegisterForPushNotification and how send an acquired device token to your server.
It turns out there’s a bit of swizzling magic happening in the background so the SDK seems to be taking care of it.
In this case I think it is at least worthwhile mentioning it and documenting the expected behaviour.
From my tests the framework captures that callback,
saved the token and then relays the message to the original app delegate method.
Is this correct?
Cheers
Rog

Yes, you are correct, we are using method swizzling to intercept method call and send the token back to our servers. This way we provide easier setup of our SDK.

@mattleanplum Hi Matt, maybe we can add this to our docs?

This is where the app delegate method gets swizzled: https://github.com/Leanplum/Leanplum-iOS-SDK/blob/develop/Leanplum-SDK/Classes/LPActionManager.m#L94
and here is our implementation: https://github.com/Leanplum/Leanplum-iOS-SDK/blob/develop/Leanplum-SDK/Classes/LPActionManager.m#L64


Dev Test: pH Scale 1.6.0-dev.6

Dev Test
Mentions: @arouinfar @kathy-phet @KatieWoe
Simulation links

github repository for issues
phet top level directory
sim: ph-scale_all_phet.html
phet-io wrapper index

Test Matrix
Please include all (non-screen reader) feature testing in these records if applicable.

[x] Latest macOS, Safari (Time = 1.5hrs)
[x] Windows 10, Chrome (Time = 1.5)
[x] Windows 10, Firefox (Time = 1.25hrs)
[x] Latest iOS, Safari (Time = 1hr)
[ ] Windows 11, Chrome (Time = )

Light testing, or optionally skip if time crunch:

[ ] Latest macOS, Chrome (Time = )
[ ] Latest Chrome OS, Chrome (Time = )

If PhET-iO is being tested:

[x] Latest macOS, Safari (Time = 3hrs)
[x] Windows 10, Chrome (Time = 3)
[x] Windows 10, Firefox (Time = 2hrs)
[x] Latest iOS, Safari (Time = 1hr)
[ ] Windows 11, Chrome (Time = )

Features included

[x] PhET-iO
Alternative Input
UI Sound
Sonification
Description
Voicing

Focus and Special Instructions
The focus of this release is PhET-iO.  General sim features have changed very little, but PhET-iO instrumentation has been brought up to current conventions.
PhET-iO migration needs to be tested against the previous version, https://phet-io.colorado.edu/sims/ph-scale/1.5/.  If you have questions about how to test migration, please contact @zepumph or @samreid.
Issues to Verify
None

For QA...

General features

What to Test

Click every single button.
If there is sound, make sure it works.
Make sure you can't lose anything.
Play with the sim normally.
Try to break the sim.
Try to include browser version numbers
If there is a console available, check for errors and include them in the Problem Description.
Run through the string tests on at least one platform, especially if it is about to go to rc.
Check the Game Up harness on one platform.

PhET-iO features

What to Test

Make sure that public files do not have password protection. Use a private browser for this.
Make sure that private files do have password protection. Use a private browser for this.
Make sure standalone sim is working properly.
Make sure the wrapper index is working properly.
Make sure each wrapper is working properly.
Launch the simulation in Studio with ?stringTest=xss and make sure the sim doesn't navigate to youtube
For newer PhET-iO wrapper indices, save the "basic example of a functional wrapper" as a .html file and open it. Make
sure the simulation loads without crashing or throwing errors.

Accessibility features

What to Test

Specific instructions can be found above.

Make sure the accessibility (a11y) feature that is being tested doesn't negatively affect the sim in any way. Here is
a list of features that may be supported in this test:

Alternative Input
Interactive Description
Sound and Sonification
Pan and Zoom
Mobile Description
Voicing

Test all possible forms of input.

Test all mouse/trackpad inputs.
Test all touchscreen inputs.
Test all keyboard navigation inputs (if applicable).
Test all forms of input with a screen reader (if applicable).

Screen Readers
This sim may support screen readers. If you are unfamiliar with screen readers, please ask Katie to introduce you to
screen readers. If you simply need a refresher on screen readers, please consult the
QA Book, which should have all of the information
you need as well as a link to a screen reader tutorial made by Jesse. Otherwise, look over the a11y view before opening
the simulation. Once you've done that, open the simulation and make sure alerts and descriptions work as intended.

Platforms and Screen Readers to Be Tested

Windows 10 + Latest Chrome + Latest JAWS
Windows 10 + Latest Firefox + Latest NVDA
macOS + Safari + VoiceOver
iOS + Safari + VoiceOver (only if specified in testing issue)

Critical Screen Reader Information
We are tracking known screen reader bugs in
here. If you find a
screen reader bug, please check it against this list.

Keyboard Navigation
This sim supports keyboard navigation. Please make sure it works as intended on all platforms by itself and with a
screen reader.
Magnification
This sim supports magnification with pinch and drag gestures on touch screens, keyboard shortcuts, and mouse/wheel controls. Please test magnfication and make sure
it is working as intended and well with the use cases of the simulation. Due to the way screen readers handle user input, magnification is NOT expected to
work while using a screen reader so there is no need to test this case.

FAQs for QA Members

There are multiple tests in this issue... Which test should I do first?
Test in order! Test the first thing first, the second thing second, and so on.

How should I format my issue?

We typically assign the developer who opened the issue in the QA repository.

My question isn't in here... What should I do?
You should:

Consult the QA Book.
Google it.
Ask Katie.
Ask a developer.
Google it again.
Cry.

QA is done

Thanks QA team.  We'll move immediately to RC testing. Closing.


User-defined preset not shown when inheriting from CMakePresets.json

Brief Issue Summary

If we have a build preset in CMakeUserPresets.json that is inheriting the configurePreset variable from another preset defined in CMakePresets.json, this preset cannot be selected and it is not shown.
Expected:

A build preset defined in CMakeUserPresets.json is shown

Apparent Behavior:
Look at the following example:

CMakePresets.json:
{
    "version": 3,
    "cmakeMinimumRequired": {
        "major": 3,
        "minor": 21,
        "patch": 0
    },
    "configurePresets": [
        {
            "name": "default",
            "displayName": "Default config",
            "binaryDir": "${sourceDir}/build/default",
            "generator": "Ninja"
        }
    ],
    "buildPresets": [
        {
            "name": "build-debug",
            "description": "Build with debug information",
            "displayName": "Build release",
            "configurePreset": "default",
            "configuration": "Debug"
        },
        {
            "name": "build-release",
            "description": "Optimized build without debug information",
            "displayName": "Build release",
            "configurePreset": "default",
            "configuration": "Release"
        }
    ]
}

CMakeUserPresets.json
{
    "version": 3,
    "cmakeMinimumRequired": {
        "major": 3,
        "minor": 21,
        "patch": 0
    },
    "buildPresets": [
        {
            "name": "VS2019",
            "description": "Use Visual Studio 2019",
            "displayName": "VS2019",
            "environment": {
                "CMAKE_PREFIX_PATH": "C:\\build\\Qt\\5.15.2\\msvc2019_64"
            }
        },
        {
            "name": "VS2019-debug",
            "displayName": "VS2019-debug",
            "inherits": ["VS2019", "build-debug"]
        },
        {
            "name": "VS2019-release",
            "displayName": "VS2019-release",
            "inherits": ["VS2019", "build-release"]
        }
    ]
}

Now, if I move either the build-debug or the build-release presets to the CMakeUserPresets.json file the VS2019-debug and VS2019-release presets are shown properly but otherwise they are not shown. This should be a valid configuration preset.
CMake Tools Log

[driver] Switching to configure preset: default
[presetController] Successfully validated presets in c:\Users\20204729\Documents\Projects\SAM-FMS\fms-scheduler\scheduler\CMakePresets.json
[presetController] Successfully validated presets in c:\Users\20204729\Documents\Projects\SAM-FMS\fms-scheduler\scheduler\CMakePresets.json
[presetController] Successfully validated presets in c:\Users\20204729\Documents\Projects\SAM-FMS\fms-scheduler\scheduler\CMakeUserPresets.json
[presetController] Successfully validated presets in c:\Users\20204729\Documents\Projects\SAM-FMS\fms-scheduler\scheduler\CMakeUserPresets.json
[driver] Switching to configure preset: default
[driver] Switching to configure preset: default

Developer Tools Log

[2021-08-26 11:36:16.026] [main] [info] window: using vscode-file:// protocol and V8 cache options: bypassHeatCheck
[2021-08-26 11:37:47.326] [main] [info] window: using vscode-file:// protocol and V8 cache options: bypassHeatCheck
[2021-08-26 11:38:50.292] [main] [info] window: using vscode-file:// protocol and V8 cache options: bypassHeatCheck
[2021-08-26 11:40:22.306] [main] [info] window: using vscode-file:// protocol and V8 cache options: bypassHeatCheck
[2021-08-26 11:41:38.444] [main] [info] window: using vscode-file:// protocol and V8 cache options: bypassHeatCheck
[2021-08-26 12:34:19.845] [main] [info] update#setState checking for updates
[2021-08-26 12:34:19.911] [main] [info] update#setState idle
[2021-08-26 13:34:19.893] [main] [info] update#setState checking for updates
[2021-08-26 13:34:20.009] [main] [info] update#setState idle

Platform and Versions

Operating System: Windows 10 19043
CMake Version: 3.21.1
VSCode Version: 1.59.1
CMake Tools Extension Version: v1.7.3
Compiler/Toolchain: Visual C++ 2019

Version 1.8.1 has this fix and was released today.


Bandwidth limit set for individual sessions

Done by throttling method, which pauses the session for the correct amount of time if the bandwidth goes over the limit

This change is

Review status: 0 of 1 files reviewed at latest revision, 8 unresolved discussions.

src/lib/rtc-to-net/rtc-to-net.ts, line 57 [r1] (raw file):

    public static SESSION_LIMIT = 10000;

    private static BANDWIDTH_LIMIT = 1000000;

Is this ever used?

src/lib/rtc-to-net/rtc-to-net.ts, line 290 [r1] (raw file):

    }

    private calculateBandwidth = () : void => {

No space after closing parenthesis here

src/lib/rtc-to-net/rtc-to-net.ts, line 360 [r1] (raw file):

    private bandwidthOverLimit :boolean = false;
    private limitBandwidth: boolean = false;

How do you imagine this variable getting used?  Also, I think we're trying to do trailing underscores for private variables but someone else can verify that.

src/lib/rtc-to-net/rtc-to-net.ts, line 363 [r1] (raw file):

    // Used to stop the calculation of bandwidth.
    private stopBandwidthCalc_ :boolean = false;

Should be stopBandwidthCalc_: boolean

src/lib/rtc-to-net/rtc-to-net.ts, line 372 [r1] (raw file):

    public static BANDWIDTH_LIMIT = 1000000;
    // 1 Mbps

Can you put the comment above the variable.
Also, this seems like something it would be helpful to have an easy way to set.
@trevj, my thought would be to make it a parameter to the constructor or start method, thoughts from your side?

src/lib/rtc-to-net/rtc-to-net.ts, line 692 [r1] (raw file):

        }
        if (!this.bandwidthOverLimit) {
          if (overflow) {

Okay, my thoughts aren't fully-formed here, but I just want to check that we've somewhat considered the case where we have a connection paused due to bandwidth, we get the overflow event, and then we end up resuming again.  Will we get a second overflow event to trigger that pause again later (and, if we don't, will the double-resume call affect anything?
Actually, same question, will anything happen if we're paused due to overflow and by some weird trick of math we then end up calling pause again due to bandwidth?

src/lib/rtc-to-net/rtc-to-net.ts, line 725 [r1] (raw file):

        var bandwidthSession_  = bitsTransferred_ / (Session.BANDWIDTH_MONITOR_INTERVAL / 1000);
        log.debug('%1: current bandwidth %2 bits/sec', this.channelLabel(), bandwidthSession_);

You have some whitespace on the empty line (did you get around to installing the typescript sublime plugin?)

src/lib/rtc-to-net/rtc-to-net.ts, line 749 [r1] (raw file):

    }

    private pauseForBandwidthOverflow = (pauseTime:number) : void => {

Spacing for types issue again.

Comments from Reviewable

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 7 unresolved discussions.

src/lib/rtc-to-net/rtc-to-net.ts, line 725 [r1] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> You have some whitespace on the empty line (did you get around to installing the typescript sublime plugin?)

And you fixed this already :)  Sorry, was looking at the old version.

Comments from Reviewable

Review status: 0 of 1 files reviewed at latest revision, 7 unresolved discussions.

src/lib/rtc-to-net/rtc-to-net.ts, line 57 [r1] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> Is this ever used?

I was using it for limiting total bandwidth, but that's on a different branch now so I'll delete it.

src/lib/rtc-to-net/rtc-to-net.ts, line 290 [r1] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> No space after closing parenthesis here

Just following what other methods did, sorry! I'll get the formatting down eventually haha :-)

src/lib/rtc-to-net/rtc-to-net.ts, line 360 [r1] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> How do you imagine this variable getting used?  Also, I think we're trying to do trailing underscores for private variables but someone else can verify that.

Lucy said that whether bandwidth gets limited or not should be set by a flag, so I created this variable!

src/lib/rtc-to-net/rtc-to-net.ts, line 363 [r1] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> Should be `stopBandwidthCalc_: boolean`

Just out of curiosity, is the space in a different place if it's a number vs a boolean? Done.

src/lib/rtc-to-net/rtc-to-net.ts, line 372 [r1] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> Can you put the comment above the variable.
>
> Also, this seems like something it would be helpful to have an easy way to set.
>
> @trevj, my thought would be to make it a parameter to the constructor or start method, thoughts from your side?

Done.

src/lib/rtc-to-net/rtc-to-net.ts, line 692 [r1] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> Okay, my thoughts aren't fully-formed here, but I just want to check that we've somewhat considered the case where we have a connection paused due to bandwidth, we get the overflow event, and then we end up resuming again.  Will we get a second overflow event to trigger that pause again later (and, if we don't, will the double-resume call affect anything?
>
> Actually, same question, will anything happen if we're paused due to overflow and by some weird trick of math we then end up calling pause again due to bandwidth?

I'm not entirely sure what you're asking, but from what I've seen from a lot of debugging, nothing weird happens if you pause twice or resume twice in a row. However, this code just prevents the overflow thing from even executing if the connection has been paused due to bandwidth, so I don't think it should even be an issue (...? again, not completely sure what you mean, so sorry if I didn't answer the question)

src/lib/rtc-to-net/rtc-to-net.ts, line 749 [r1] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> Spacing for types issue again.

Done.

Comments from Reviewable

OK, promising start.
I set limitBandwidth to true (would be good to mention this is necessary in the PR description, BTW) and ran this command:
curl -O -C - -x socks5h://<IP_ADDRESS>:9999 http://mirror.math.princeton.edu/pub/ubuntu-iso/16.04/ubuntu-16.04-desktop-amd64.iso

Having played with the parameters a bit (5s is much too long an interval), my bandwidth is limited but drops to zero and never recovers. Have you successfully ran the above command and seen throughput stabilising to the configured value?

@trevj I think I figured out a better way to pause/resume the connection; I'll talk to you about it

Review status: 0 of 1 files reviewed at latest revision, 11 unresolved discussions.

src/lib/rtc-to-net/rtc-to-net.ts, line 360 [r1] (raw file):
Previously, shilpa10kumar9 wrote…
> Lucy said that whether bandwidth gets limited or not should be set by a flag, so I created this variable!

Yes! That was me! Trailing underspaces (limitBandwidth_) are helpful for us to identify private variables!

src/lib/rtc-to-net/rtc-to-net.ts, line 363 [r1] (raw file):
Previously, shilpa10kumar9 wrote…
> Just out of curiosity, is the space in a different place if it's a number vs a boolean? Done.

Looking online it seems more popular to have the space after the colon, but as long as we keep our style consistent either works!

src/lib/rtc-to-net/rtc-to-net.ts, line 692 [r1] (raw file):
Previously, shilpa10kumar9 wrote…
> I'm not entirely sure what you're asking, but from what I've seen from a lot of debugging, nothing weird happens if you pause twice or resume twice in a row. However, this code just prevents the overflow thing from even executing if the connection has been paused due to bandwidth, so I don't think it should even be an issue (...? again, not completely sure what you mean, so sorry if I didn't answer the question)

Is it possible to call this.tcpConnection_.pause(); when the connection is already paused? If that doesn't cause bad behaviour, it will be good practice to avoid doing that. Maybe we can rename this.bandwidthOverLimit_ to pausedForBandwidthOverflow_ and add a comment that we cant overflow if we are already paused (if that is in fact true). That would make the behaviour more clear to me! Thoughts @jpevarnek?

src/lib/rtc-to-net/rtc-to-net.ts, line 725 [r1] (raw file):

        var bandwidthSession_  = bitsTransferred_ / (Session.BANDWIDTH_MONITOR_INTERVAL / 1000);
        log.debug('%1: current bandwidth %2 bits/sec', this.channelLabel(), bandwidthSession_);

nit: extra spaces here

src/lib/rtc-to-net/rtc-to-net.ts, line 760 [r1] (raw file):

      this.tcpConnection_.resume();
    }

nit: extra spaces

src/lib/rtc-to-net/rtc-to-net.ts, line 361 [r5] (raw file):

    private limitBandwidth_: boolean = true;

    private notPausedFraction_ :number = 1;

nit: add a comment that 1 = don't pause at all?

src/lib/rtc-to-net/rtc-to-net.ts, line 375 [r5] (raw file):

    // is not accurately limited. In other words, even though the connection is paused,
    // the consequence of a very short pause is that many bits are transferred in the
    //time it takes to call the method again, which ncreases the bandwidth.

nit: missing a space before "time" and spelling for "increases"
Also to make sure I understand: do you mean that for short pauses, many bits will be transferred in the brief moments between pauses/resumes?

src/lib/rtc-to-net/rtc-to-net.ts, line 730 [r5] (raw file):

        // Calculated in milliseconds.
        var notPausedTime = Session.BANDWIDTH_MONITOR_INTERVAL * this.notPausedFraction_;
        // Bandwidth is how many bytes were sent/received over time that WASN'T paused, since a fraction of every interval is paused.

A fraction of every interval is paused, except when notPausedFraction_ = 1, is that right?

src/lib/rtc-to-net/rtc-to-net.ts, line 741 [r5] (raw file):

        if (this.limitBandwidth_) {
          if (bandwidthSession > Session.BANDWIDTH_LIMIT) {
            this.bandwidthOverLimit_ = true;

If we rename this to pausedForBandwidthOverflow_ I think we should move this line into the pause function

src/lib/rtc-to-net/rtc-to-net.ts, line 749 [r5] (raw file):

            this.pauseForBandwidthOverflow(timeToPause);
          } else {
            // Reset the fraction to not pause during to 1; do not pause during the next interval.

nit: possibly more clear: "Reset the fraction of time not paused to 1;..."

Comments from Reviewable

Aah, looks like I forgot to actually submit comments yesterday.

Reviewed 1 of 1 files at r3, 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 18 unresolved discussions.

src/lib/rtc-to-net/rtc-to-net.ts, line 360 [r1] (raw file):
Previously, shilpa10kumar9 wrote…
> Lucy said that whether bandwidth gets limited or not should be set by a flag, so I created this variable!

Okay, it might be better to have the flag be something that could be toggled either at runtime or a single place for the entire file.  Having a class variable serving as a compile-time flag feels a bit off instinctively.

src/lib/rtc-to-net/rtc-to-net.ts, line 363 [r1] (raw file):
Previously, shilpa10kumar9 wrote…
> Just out of curiosity, is the space in a different place if it's a number vs a boolean? Done.

Nope, should always be `variable: type` (we were ignoring that style point for a while)
https://github.com/Microsoft/TypeScript/wiki/Coding-guidelines has most of the rules (we ignore some for various reasons)

src/lib/rtc-to-net/rtc-to-net.ts, line 692 [r1] (raw file):
Previously, shilpa10kumar9 wrote…
> I'm not entirely sure what you're asking, but from what I've seen from a lot of debugging, nothing weird happens if you pause twice or resume twice in a row. However, this code just prevents the overflow thing from even executing if the connection has been paused due to bandwidth, so I don't think it should even be an issue (...? again, not completely sure what you mean, so sorry if I didn't answer the question)

I'm just a bit concerned that it seems like we might be playing a bit loose with knowing exactly what state we're in.  I think we pretty much have four states:

Under limit, no overflow (unpaused)
Over limit, no overflow (paused for some interval)
Under limit, overflow (paused)
Over limit, overflow (paused)

and I want to make sure that we're very sure about which state we're in and how we're switching between them.  Calling functions and relying on their behaviour to do nothing if we're in the wrong state just makes me worried that that functionality will get changed somehow and we'll end up with different behaviour.

src/lib/rtc-to-net/rtc-to-net.ts, line 749 [r1] (raw file):
Previously, shilpa10kumar9 wrote…
> Done.

Same thing for the function (`): void`)

src/lib/rtc-to-net/rtc-to-net.ts, line 361 [r4] (raw file):

    private limitBandwidth_: boolean = true;

    private notPausedFraction_ :number = 1;

nit: type spacing

src/lib/rtc-to-net/rtc-to-net.ts, line 375 [r4] (raw file):

    // is not accurately limited. In other words, even though the connection is paused,
    // the consequence of a very short pause is that many bits are transferred in the
    //time it takes to call the method again, which ncreases the bandwidth.

nit: leading space before t
Also, missing an i on increases
Also trailing whitespace on second-to-last line

src/lib/rtc-to-net/rtc-to-net.ts, line 736 [r4] (raw file):

        log.debug('%1: Bandwidth over whole interval (should be close to limit, or under) = %2', this.channelLabel(), bandwidthWholeInterval);
        /*
          Bandwidth limiting starts here. If the current bandwidth is over the limit, the fraction of the next interval to pause

For this comment, was there a specific line length you were aiming for?  Also, as a personal preference, I like an * at the beginning of each line in a block comment

src/lib/rtc-to-net/rtc-to-net.ts, line 706 [r5] (raw file):

          }
        }

Unnecessary blank line?

src/lib/rtc-to-net/rtc-to-net.ts, line 725 [r5] (raw file):

    // average is still low.
    private calculateBandwidth_ = () : void => {
      if (!this.stopBandwidthCalc_) {

We can get rid of a level of indentation if you switch the structure to:
if (this.stopBandwidthCalc_) {
  return;
}

src/lib/rtc-to-net/rtc-to-net.ts, line 743 [r5] (raw file):

            this.bandwidthOverLimit_ = true;
            // Calculate correct fraction of time interval to not pause the session during.
            this.notPausedFraction_ = Session.BANDWIDTH_LIMIT / bandwidthSession;

It looks like we're relying on the assumption that if we have the connection paused for exactly half the interval, the bandwidth will be exactly half of what it would be without the pause.  Is that verified? (sorry to be so nit-y, I'm sure that you talked about this with Trevor but I haven't had a chance to experiment and want to double-check)

src/lib/rtc-to-net/rtc-to-net.ts, line 753 [r5] (raw file):

          }
          this.prevBytes_ = this.currBytes_;
          setTimeout(this.calculateBandwidth_, Session.BANDWIDTH_MONITOR_INTERVAL);

This may be a reviewable issue, but it looks to me like we only set the timeout if limitBandwidth_ is true, can you confirm (and fix if necessary)?

Comments from Reviewable

Review status: 0 of 1 files reviewed at latest revision, 18 unresolved discussions.

src/lib/rtc-to-net/rtc-to-net.ts, line 360 [r1] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> Okay, it might be better to have the flag be something that could be toggled either at runtime or a single place for the entire file.  Having a class variable serving as a compile-time flag feels a bit off instinctively.

What exactly do you mean by having it in a single place for the entire file? Should I move it to the ```RtcToNet``` class in this file?

src/lib/rtc-to-net/rtc-to-net.ts, line 363 [r1] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> Nope, should always be `variable: type` (we were ignoring that style point for a while)
>
> https://github.com/Microsoft/TypeScript/wiki/Coding-guidelines has most of the rules (we ignore some for various reasons)

Done.

src/lib/rtc-to-net/rtc-to-net.ts, line 692 [r1] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> I'm just a bit concerned that it seems like we might be playing a bit loose with knowing exactly what state we're in.  I think we pretty much have four states:
>
> * Under limit, no overflow (unpaused)
> * Over limit, no overflow (paused for some interval)
> * Under limit, overflow (paused)
> * Over limit, overflow (paused)
>
> and I want to make sure that we're very sure about which state we're in and how we're switching between them.  Calling functions and relying on their behaviour to do nothing if we're in the wrong state just makes me worried that that functionality will get changed somehow and we'll end up with different behaviour.

Lucy and I talked a lot about this, and we came up with a solution:
Have two flags, ```pausedForBandwidthOverflow``` and ```pausedForChannelOverflow```. Calculate time needed to pause for bandwidth overflow just like normal, and set an interval to call the resume method after that amount of time; just don't do ```this.tcpConnection_.pause()``` if it's paused for overflow, and don't do ```this.tcpConnection_.resume()``` if it's still paused for overflow. And same thing switched for overflow pausing/resuming.c
Sorry if my explanation doesn't make sense; I can go into more detail if you'd like!

src/lib/rtc-to-net/rtc-to-net.ts, line 725 [r1] (raw file):
Previously, lucyhe wrote…
> nit: extra spaces here

I actually don't see the spaces!

src/lib/rtc-to-net/rtc-to-net.ts, line 749 [r1] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> Same thing for the function (`): void`)

Done.

src/lib/rtc-to-net/rtc-to-net.ts, line 760 [r1] (raw file):
Previously, lucyhe wrote…
> nit: extra spaces

Still don't see the spaces! Maybe a reviewable issue?

src/lib/rtc-to-net/rtc-to-net.ts, line 361 [r4] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> nit: type spacing

Done.

src/lib/rtc-to-net/rtc-to-net.ts, line 375 [r4] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> nit: leading space before t
> Also, missing an i on increases
> Also trailing whitespace on second-to-last line

Done.

src/lib/rtc-to-net/rtc-to-net.ts, line 736 [r4] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> For this comment, was there a specific line length you were aiming for?  Also, as a personal preference, I like an `*` at the beginning of each line in a block comment

No, not really, but I'll try to make it more uniform! Also, done.

src/lib/rtc-to-net/rtc-to-net.ts, line 361 [r5] (raw file):
Previously, lucyhe wrote…
> nit: add a comment that 1 = don't pause at all?

Done.

src/lib/rtc-to-net/rtc-to-net.ts, line 375 [r5] (raw file):
Previously, lucyhe wrote…
> nit: missing a space before "time" and spelling for "increases"
> Also to make sure I understand: do you mean that for short pauses, many bits will be transferred in the brief moments between pauses/resumes?
>

Yes, that's exactly what I mean! It makes the subsequent bandwidth calculation a lot less accurate.

src/lib/rtc-to-net/rtc-to-net.ts, line 706 [r5] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> Unnecessary blank line?

Done.

src/lib/rtc-to-net/rtc-to-net.ts, line 725 [r5] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> We can get rid of a level of indentation if you switch the structure to:
> ```
> if (this.stopBandwidthCalc_) {
>   return;
> }
> ```

Done.

src/lib/rtc-to-net/rtc-to-net.ts, line 730 [r5] (raw file):
Previously, lucyhe wrote…
> A fraction of every interval is paused, except when notPausedFraction_ = 1, is that right?

yes! I'll make the comment clearer.

src/lib/rtc-to-net/rtc-to-net.ts, line 741 [r5] (raw file):
Previously, lucyhe wrote…
> If we rename this to pausedForBandwidthOverflow_ I think we should move this line into the pause function

Done.

src/lib/rtc-to-net/rtc-to-net.ts, line 743 [r5] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> It looks like we're relying on the assumption that if we have the connection paused for exactly half the interval, the bandwidth will be exactly half of what it would be without the pause.  Is that verified? (sorry to be so nit-y, I'm sure that you talked about this with Trevor but I haven't had a chance to experiment and want to double-check)

From my experimenting, it all works out! I tried with simpleSocks as well as a normal peer-to-peer connection.
Here's the math:
Let's say that the bandwidth limit is 50, but we measure the bandwidth to be 100. You want to pause a fraction of the next interval: 50 = 100f, where f is the fraction that you do not want to pause. f = 1/2. For the next interval, if the interval is 5 seconds, you have ``` (100(1/2 * 5) + 0 (1/2*5))/5 = 50 ```, so the bandwidth turns out to be exactly half of what it would be without the pause.

src/lib/rtc-to-net/rtc-to-net.ts, line 749 [r5] (raw file):
Previously, lucyhe wrote…
> nit: possibly more clear: "Reset the fraction of time not paused to 1;..."

Done.

src/lib/rtc-to-net/rtc-to-net.ts, line 753 [r5] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> This may be a reviewable issue, but it looks to me like we only set the timeout if `limitBandwidth_` is true, can you confirm (and fix if necessary)?

 I didn't even catch it cause ```limitBandwidth_``` was always true!

Comments from Reviewable

Review status: 0 of 1 files reviewed at latest revision, 15 unresolved discussions.

src/lib/rtc-to-net/rtc-to-net.ts, line 360 [r1] (raw file):
Previously, shilpa10kumar9 wrote…
> What exactly do you mean by having it in a single place for the entire file? Should I move it to the ```RtcToNet``` class in this file?

I think moving into RtcToNet sounds good. I'm sorry that I missed that it was in Session! Deferring to @jpevarnek though

src/lib/rtc-to-net/rtc-to-net.ts, line 725 [r1] (raw file):
Previously, shilpa10kumar9 wrote…
> I actually don't see the spaces!

Sorry my bad! Was looking at an older version!

src/lib/rtc-to-net/rtc-to-net.ts, line 760 [r1] (raw file):
Previously, shilpa10kumar9 wrote…
> Still don't see the spaces! Maybe a reviewable issue?

Yes sorry!! My mistake!

src/lib/rtc-to-net/rtc-to-net.ts, line 736 [r4] (raw file):
Previously, shilpa10kumar9 wrote…
> No, not really, but I'll try to make it more uniform! Also, done.

Could we line up the asterisks like they are here: https://github.com/uProxy/uproxy/blob/master/src/generic_ui/scripts/background_ui.ts#L175

src/lib/rtc-to-net/rtc-to-net.ts, line 377 [r7] (raw file):

    // is not accurately limited. In other words, even though the connection is paused,
    // the consequence of a very short pause is that many bits are transferred in the
    // time it takes to call the method again, which increases the bandwidth.da

I'm seeing "bandwidth.da" I think should be "bandwidth"

src/lib/rtc-to-net/rtc-to-net.ts, line 701 [r7] (raw file):

        this.pausedForChannelOverflow_ = overflow;
        // Need to pause for overflow.
        if (this.pausedForChannelOverflow_) {

nit-y: I think the code reads a little better if we don't set pausedForChannelOverflow_ until after we perform a pause or resume action. So, I think in this line we can have if (overflow) and then move this.pausedForChannelOverflow_ = overflow to the bottom of this function. That way we will definitely call pause() before setting pausedForChannelOverflow_ to true.

Comments from Reviewable

Review status: 0 of 1 files reviewed at latest revision, 15 unresolved discussions.

src/lib/rtc-to-net/rtc-to-net.ts, line 360 [r1] (raw file):
Previously, lucyhe wrote…
> I think moving into RtcToNet sounds good. I'm sorry that I missed that it was in Session! Deferring to @jpevarnek though

Done.

src/lib/rtc-to-net/rtc-to-net.ts, line 736 [r4] (raw file):
Previously, lucyhe wrote…
> Could we line up the asterisks like they are here: https://github.com/uProxy/uproxy/blob/master/src/generic_ui/scripts/background_ui.ts#L175

Done.

src/lib/rtc-to-net/rtc-to-net.ts, line 377 [r7] (raw file):
Previously, lucyhe wrote…
> I'm seeing "bandwidth.da" I think should be "bandwidth"

Done.

src/lib/rtc-to-net/rtc-to-net.ts, line 701 [r7] (raw file):
Previously, lucyhe wrote…
> nit-y: I think the code reads a little better if we don't set pausedForChannelOverflow_ until after we perform a pause or resume action. So, I think in this line we can have `if (overflow)` and then move `this.pausedForChannelOverflow_ = overflow` to the bottom of this function. That way we will definitely call pause() before setting pausedForChannelOverflow_ to true.

Done.

Comments from Reviewable

Two nits, but looks good. When you check it in can you set LIMIT_BANDWIDTH to false? Until this is configurable by the user I don't htink we want their bandwidth to be limited!

Review status: 0 of 1 files reviewed at latest revision, 16 unresolved discussions.

src/lib/rtc-to-net/rtc-to-net.ts, line 359 [r8] (raw file):

    private pausedForBandwidthOverflow_: boolean = false;
    //private limitBandwidth_: boolean = true;

I'm seeing limitBandwidth here as a comment, could you make sure its taken out (not sure if reviewable issue!)

src/lib/rtc-to-net/rtc-to-net.ts, line 794 [r8] (raw file):

    private isAllowedAddress_ = (addressString:string) : boolean => {
      // default is to disallow non-unicast addresses; i.e. only proxy forc

Extra "c" was added at the end of this line

Comments from Reviewable

Review status: 0 of 1 files reviewed at latest revision, 16 unresolved discussions.

src/lib/rtc-to-net/rtc-to-net.ts, line 359 [r8] (raw file):
Previously, lucyhe wrote…
> I'm seeing limitBandwidth here as a comment, could you make sure its taken out (not sure if reviewable issue!)

Done.

src/lib/rtc-to-net/rtc-to-net.ts, line 794 [r8] (raw file):
Previously, lucyhe wrote…
> Extra "c" was added at the end of this line

wasn't me haha but Done! :)

Comments from Reviewable

:+1: from me! Good stuff!

@trevj For testing, I did each of the following steps for both SimpleSocks + curl command, and a normal peer-to-peer connection + loading a youtube video (etc).

I compared the times for the command to complete with the bandwidthLimit on vs off (limit on should take a lot longer).
I tested with curl and made sure the download speed was close to the limit. It rarely came to the exact number but was always close. To convert, if the number is 100K, do 10010248 to get the number in bps, which is the limit's units
With both curl and the peer-to-peer, I tested with different limits and compared times it took for curl to finish/a page to load. Again, the lower the bandwidth limit, the longer it should take.
Used background.html and the debug statements to verify the math of everything-- made sure it was pausing for the right amount of the next interval to make the bandwidth as close to the limit, etc.

General question I probably should have asked sooner: setTimeout is guaranteed to resume at precisely Xms after it was called, is there any chance that we could be getting bad values from that?
I think that this is something where it would be very useful to have a quick design review as a team over the method of pausing just to make sure we have the right algorithm and aren't missing any other ways to do it.  Even if all it is is a single slide that has a description of the algorithm:

Guess the per-session bandwidth limit based on total/num sessions
Loop through all sessions, add up unused bandwidth by sessions under limit and also calculate total bandwidth
new per-session bandwidth = total / num sessions + unused / sessions over limit
Loop through all sessions again, pause any session over per-session limit by an amount directly proportional to how much over the limit they are

Reviewed 1 of 2 files at r20, 4 of 4 files at r21.
Review status: all files reviewed at latest revision, 19 unresolved discussions, all commit checks successful.

src/interfaces/uproxy_core_api.ts, line 102 [r21] (raw file):

}

export interface bandwidthSettings {

Interface name should be capitalized

src/lib/rtc-to-net/bandwidth-config.ts, line 3 [r21] (raw file):

import uproxy_core_api = require('../../interfaces/uproxy_core_api');

interface BandwidthConfig {

I may have missed a bit of context here, but is there a specific reason we have this file?

src/lib/rtc-to-net/bandwidth-config.ts, line 7 [r21] (raw file):

}

export = BandwidthConfig;

It looks like you have a trailing space here, should be a trailing blank line instead

src/lib/rtc-to-net/rtc-to-net.ts, line 743 [r5] (raw file):
Previously, shilpa10kumar9 wrote…
> From my experimenting, it all works out! I tried with simpleSocks as well as a normal peer-to-peer connection.
> Here's the math:
> Let's say that the bandwidth limit is 50, but we measure the bandwidth to be 100. You want to pause a fraction of the next interval: 50 = 100f, where f is the fraction that you do not want to pause. f = 1/2. For the next interval, if the interval is 5 seconds, you have ``` (100(1/2 * 5) + 0 (1/2*5))/5 = 50 ```, so the bandwidth turns out to be exactly half of what it would be without the pause.

Okay, great if it worked out experimentally.
That's assuming that what's limiting the bandwidth is solely the amount of time that this socket is able to receive traffic.  The limit could be coming from many other sources though.  Also a tad confused that it would have strange interactions with TCP window algorithms.

src/lib/rtc-to-net/rtc-to-net.ts, line 78 [r21] (raw file):

    private stopBandwidthCalc: boolean = false;
    private prevBytes: number = 0;
    private static limitBandwidth: boolean = true;

This should probably default to false, no?
And on a side note, probably would be good to keep all the static variables together in the file so they can quickly be found.  Up near the top above instance variables would probably be good.

src/lib/rtc-to-net/rtc-to-net.ts, line 79 [r21] (raw file):

    private prevBytes: number = 0;
    private static limitBandwidth: boolean = true;
    private static bandwidthLimit: number = 1000000;

Can you add a comment explaining what this is limited based on (i.e. bandwidth/session, bandwidth/user, etc.)

src/lib/rtc-to-net/rtc-to-net.ts, line 222 [r21] (raw file):

    public static updateBandwidthSettings = (
      bandwidthConfig: BandwidthConfig) => {

This should either be on the same line as the above (I think that will work) or should be indented four spaces compared to the previous line.

src/lib/rtc-to-net/rtc-to-net.ts, line 322 [r21] (raw file):

    private calculateBandwidth = (): void => {
      if (!this.stopBandwidthCalc) {

It would be good if you could switch this to:
if (this.stopBandwidthCalc) {
  return;
}

to get rid of the extra level of indentation.

src/lib/rtc-to-net/rtc-to-net.ts, line 326 [r21] (raw file):

        var bufferBandwidth = 0;
        var sessionsOverLimit = 0;
        var numSessions = Object.keys(this.sessions_).length;

Can you use _.size here?  This is really simple, but it's also nice to use simple helper methods like that so you get the improvement if best-practice changes.

src/lib/rtc-to-net/rtc-to-net.ts, line 330 [r21] (raw file):

        if (numSessions == 0) {
          setTimeout(this.calculateBandwidth, RtcToNet.BANDWIDTH_MONITOR_INTERVAL);
          return;

This feels like a premature optimization to me.  I'm pretty sure you'll just skip the for loop and then do nothing in the other statement if you don't have that.

src/lib/rtc-to-net/rtc-to-net.ts, line 331 [r21] (raw file):

          setTimeout(this.calculateBandwidth, RtcToNet.BANDWIDTH_MONITOR_INTERVAL);
          return;
        }

Add a blank line after the closing bracket here.

src/lib/rtc-to-net/rtc-to-net.ts, line 337 [r21] (raw file):

        for (var label in this.sessions_) {
          // Bandwidth is measured in bps.
          if (this.sessions_[label].prevBytes_ == null) {

If you're commonly referring to this.sessions_[label], probably better to have let session = this.sessions_[label] in the loop to simplify things.

src/lib/rtc-to-net/rtc-to-net.ts, line 354 [r21] (raw file):

          }
          // If the bandwidth of this session is more than the allowed limit per session, add to sessionsOverLimit.
          else if (bandwidthSession > perSessionBandwidthLimit) {

This should be on the same line as the previous closing brace.

src/lib/rtc-to-net/rtc-to-net.ts, line 356 [r21] (raw file):

          else if (bandwidthSession > perSessionBandwidthLimit) {
            sessionsOverLimit++;
          }

Blank line after the closing brace

src/lib/rtc-to-net/rtc-to-net.ts, line 358 [r21] (raw file):

          }
          // Update prevBytes_ of this session.
          this.sessions_[label].prevBytes_ = this.sessions_[label].currBytes_;

If we're accessing this outside of the session, it should be a public variable with the interface defined (seems a bit of an odd api for a class).
And, just to double-check, this is no longer being updated in the session code?

src/lib/rtc-to-net/rtc-to-net.ts, line 369 [r21] (raw file):

            // If there is any buffer bandwidth, split that evenly among sessions that went over the limit.
            // If the total went over the limit, sessionsOverLimit has to have at least 1 session in it.
            perSessionBandwidthLimit += bufferBandwidth / (sessionsOverLimit);

Parentheses here are unnecessary

src/lib/rtc-to-net/rtc-to-net.ts, line 450 [r21] (raw file):

    // Records the bytes sent to and from peer, for the current time interval.
    public currBytes_: number = 0;

If public, should not have underscore.

Comments from Reviewable

Review status: 2 of 5 files reviewed at latest revision, 19 unresolved discussions, some commit checks pending.

src/interfaces/uproxy_core_api.ts, line 102 [r21] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> Interface name should be capitalized

Done.

src/lib/rtc-to-net/bandwidth-config.ts, line 3 [r21] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> I may have missed a bit of context here, but is there a specific reason we have this file?

I was modeling my code after the way proxySettings is passed from the core to ```rtc-to-net```. For some reason, calling ```uproxy_core_api.bandwidthSettings``` directly from ```rtc-to-net``` does not work, and you need this intermediate class.

src/lib/rtc-to-net/bandwidth-config.ts, line 7 [r21] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> It looks like you have a trailing space here, should be a trailing blank line instead

Done.

src/lib/rtc-to-net/rtc-to-net.ts, line 78 [r21] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> This should probably default to false, no?
>
> And on a side note, probably would be good to keep all the static variables together in the file so they can quickly be found.  Up near the top above instance variables would probably be good.

Done.

src/lib/rtc-to-net/rtc-to-net.ts, line 79 [r21] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> Can you add a comment explaining what this is limited based on (i.e. bandwidth/session, bandwidth/user, etc.)

Done.

src/lib/rtc-to-net/rtc-to-net.ts, line 222 [r21] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> This should either be on the same line as the above (I think that will work) or should be indented four spaces compared to the previous line.

Done.

src/lib/rtc-to-net/rtc-to-net.ts, line 322 [r21] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> It would be good if you could switch this to:
> ```
> if (this.stopBandwidthCalc) {
>   return;
> }
> ```
> to get rid of the extra level of indentation.

Done.

src/lib/rtc-to-net/rtc-to-net.ts, line 326 [r21] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> Can you use `_.size` here?  This is really simple, but it's also nice to use simple helper methods like that so you get the improvement if best-practice changes.

It's giving me an error every time I try to replace ```.length``` with ```._size```!

src/lib/rtc-to-net/rtc-to-net.ts, line 330 [r21] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> This feels like a premature optimization to me.  I'm pretty sure you'll just skip the for loop and then do nothing in the other statement if you don't have that.

Yup, it'll skip the for loop, but this is so that you don't have a divide-by-zero error on the next line.

src/lib/rtc-to-net/rtc-to-net.ts, line 331 [r21] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> Add a blank line after the closing bracket here.

Done.

src/lib/rtc-to-net/rtc-to-net.ts, line 337 [r21] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> If you're commonly referring to `this.sessions_[label]`, probably better to have `let session = this.sessions_[label]` in the loop to simplify things.

Done.

src/lib/rtc-to-net/rtc-to-net.ts, line 354 [r21] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> This should be on the same line as the previous closing brace.

Done.

src/lib/rtc-to-net/rtc-to-net.ts, line 356 [r21] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> Blank line after the closing brace

Done.

src/lib/rtc-to-net/rtc-to-net.ts, line 358 [r21] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> If we're accessing this outside of the session, it should be a public variable with the interface defined (seems a bit of an odd api for a class).
>
> And, just to double-check, this is no longer being updated in the session code?

Yup, ```prevBytes``` no longer being updated.

src/lib/rtc-to-net/rtc-to-net.ts, line 369 [r21] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> Parentheses here are unnecessary

Done.

src/lib/rtc-to-net/rtc-to-net.ts, line 450 [r21] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> If public, should not have underscore.

Done.

Comments from Reviewable

Reviewed 2 of 3 files at r22.
Review status: 4 of 5 files reviewed at latest revision, 21 unresolved discussions, all commit checks successful.

src/lib/rtc-to-net/bandwidth-config.ts, line 3 [r21] (raw file):
Previously, shilpa10kumar9 wrote…
> I was modeling my code after the way proxySettings is passed from the core to ```rtc-to-net```. For some reason, calling ```uproxy_core_api.bandwidthSettings``` directly from ```rtc-to-net``` does not work, and you need this intermediate class.

In that case, it would be much better to have this be the type we actually user (possibly just exporting it directly from `rtc-to-net`) and use that in the core api.

src/lib/rtc-to-net/rtc-to-net.ts, line 78 [r21] (raw file):
Previously, shilpa10kumar9 wrote…
> Done.

(still showing up as true)

src/lib/rtc-to-net/rtc-to-net.ts, line 326 [r21] (raw file):
Previously, shilpa10kumar9 wrote…
> It's giving me an error every time I try to replace ```.length``` with ```._size```!

I'm sorry, clarification:
`_` (lodash) is a library with a ton of useful utility function.  One of those is size.  I'm saying that using `_.size` would be a more consistent way to check the number of keys, not that the `_size` method should be called.

src/lib/rtc-to-net/rtc-to-net.ts, line 330 [r21] (raw file):
Previously, shilpa10kumar9 wrote…
> Yup, it'll skip the for loop, but this is so that you don't have a divide-by-zero error on the next line.

Ideally we'd still want to see that log statement then I'd assume.  This seems like a problem in the math for the log statement, not something that should be optimized out.

src/lib/rtc-to-net/rtc-to-net.ts, line 358 [r21] (raw file):
Previously, shilpa10kumar9 wrote…
> Yup, ```prevBytes``` no longer being updated.

Slight code smell still with updating another classes properties solely for us to store data

src/lib/rtc-to-net/rtc-to-net.ts, line 323 [r23] (raw file):

      if (this.stopBandwidthCalc) {
        return;
      }

Blank line after the closing brace.

src/lib/rtc-to-net/rtc-to-net.ts, line 340 [r23] (raw file):

        let session = this.sessions_[label];
        // Bandwidth is measured in bps.
        if (session.prevBytes == null) {

Should prefer === unless a specific reason not to use it

src/lib/rtc-to-net/rtc-to-net.ts, line 348 [r23] (raw file):

          if (session.pauseR < session.pauseStart) {
            var diffAtEnd = new Date().getTime() - session.pauseStart;
            session.additionalTime += diffAtEnd;

We have incredibly closely duplicated code between the two cases here, any way to simplify that a bit?

src/lib/rtc-to-net/rtc-to-net.ts, line 363 [r23] (raw file):

          // Make sure bandwidth is calculated based on the time in last interval NOT paused, for either bandwidth or socket overflow.
          var bandwidthSession = bitsInterval / (((RtcToNet.BANDWIDTH_MONITOR_INTERVAL * session.notPausedFrac)-session.additionalTime)/ 1000);
        }

blank line after this

src/lib/rtc-to-net/rtc-to-net.ts, line 369 [r23] (raw file):

        if (bandwidthSession < perSessionBandwidthLimit) {
          bufferBandwidth += (perSessionBandwidthLimit - bandwidthSession);
        } else if (bandwidthSession > perSessionBandwidthLimit) { // If the bandwidth of this session is more than the allowed limit per session, add to sessionsOverLimit.

Would be cleaner to make the if statement use <== and switch this back to an else.

src/lib/rtc-to-net/rtc-to-net.ts, line 485 [r23] (raw file):

    public pauseStart: number = 0;
    // The last time the session was resumed after socket overflow.
    public pauseR: number = 0;

Can you use a clearer variable name here?

src/lib/rtc-to-net/rtc-to-net.ts, line 838 [r23] (raw file):

          // Check if connection is not already paused for bandwidth overflow.
          if (!this.pausedForBandwidthOverflow_) {
            this.tcpConnection_.pause();

Any possible usage out of a function that handles pausing and setting the time?

src/lib/rtc-to-net/rtc-to-net.ts, line 842 [r23] (raw file):

            this.pauseStart = new Date().getTime();
            log.debug('pause f: ' + this.pauseStart);
          } else {

Can you make this a much clearer message?  Thanks.

src/lib/rtc-to-net/rtc-to-net.ts, line 844 [r23] (raw file):

          } else {
            log.debug('%1: Hit overflow, but connection is already paused', this.longId());
            this.pauseStart = 0;

Why do we overwrite the value here?

src/lib/rtc-to-net/rtc-to-net.ts, line 852 [r23] (raw file):

            this.tcpConnection_.resume();
            log.debug('%1: Exited  overflow, resuming socket', this.longId());
            if (this.pauseStart > 0) {

What is this checking?

src/lib/rtc-to-net/rtc-to-net.ts, line 854 [r23] (raw file):

            if (this.pauseStart > 0) {
              this.pauseR = new Date().getTime();
              this.additionalTime += (this.pauseR - this.pauseStart);

Unnecessary parentheses

src/lib/rtc-to-net/rtc-to-net.ts, line 855 [r23] (raw file):

              this.pauseR = new Date().getTime();
              this.additionalTime += (this.pauseR - this.pauseStart);
              log.debug('pause f: ' + this.pauseStart + ' ; pause r: ' + this.pauseR);

Can you make this message clearer?

Comments from Reviewable

Review status: 3 of 4 files reviewed at latest revision, 21 unresolved discussions, some commit checks pending.

src/lib/rtc-to-net/rtc-to-net.ts, line 78 [r21] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> (still showing up as true)

ah! so sorry, switched it back to true for testing with simpleSocks. Done.

src/lib/rtc-to-net/rtc-to-net.ts, line 326 [r21] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> I'm sorry, clarification:
> `_` (lodash) is a library with a ton of useful utility function.  One of those is size.  I'm saying that using `_.size` would be a more consistent way to check the number of keys, not that the `_size` method should be called.

Do I need to import something? It's still not working (but I'm probably doing something wrong)

src/lib/rtc-to-net/rtc-to-net.ts, line 330 [r21] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> Ideally we'd still want to see that log statement then I'd assume.  This seems like a problem in the math for the log statement, not something that should be optimized out.

Not entirely sure what you mean, but I added another log statement for the 0 case.

src/lib/rtc-to-net/rtc-to-net.ts, line 358 [r21] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> Slight code smell still with updating another classes properties solely for us to store data

Another solution is to have some sort of map that maps sessions to prevBytes, currBytes, and all other necessary variables...is that better? I think this way is a little cleaner but I can change it if necessary!

src/lib/rtc-to-net/rtc-to-net.ts, line 323 [r23] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> Blank line after the closing brace.

Done.

src/lib/rtc-to-net/rtc-to-net.ts, line 340 [r23] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> Should prefer `===` unless a specific reason not to use it

Done.

src/lib/rtc-to-net/rtc-to-net.ts, line 348 [r23] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> We have incredibly closely duplicated code between the two cases here, any way to simplify that a bit?

Done.

src/lib/rtc-to-net/rtc-to-net.ts, line 363 [r23] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> blank line after this

Done.

src/lib/rtc-to-net/rtc-to-net.ts, line 369 [r23] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> Would be cleaner to make the if statement use `<==` and switch this back to an else.

Done.

src/lib/rtc-to-net/rtc-to-net.ts, line 485 [r23] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> Can you use a clearer variable name here?

Done.

src/lib/rtc-to-net/rtc-to-net.ts, line 838 [r23] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> Any possible usage out of a function that handles pausing and setting the time?

Well, there are so many edge cases and different ways that the connection is paused/not paused/resumed etc, I am not sure if moving this to a separate method will be entirely useful..

src/lib/rtc-to-net/rtc-to-net.ts, line 842 [r23] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> Can you make this a much clearer message?  Thanks.

Done.

src/lib/rtc-to-net/rtc-to-net.ts, line 844 [r23] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> Why do we overwrite the value here?

We overwrite the value to 0 because the start time for the socket being paused was called when the socket was already paused for bandwidth overflow. When the socket later resumes, you don't want to add the paused time twice (once for bandwidth and once for overflow)

src/lib/rtc-to-net/rtc-to-net.ts, line 852 [r23] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> What is this checking?

This is checking whether the connection was paused during the previous interval or during a time paused for bandwidth overflow. In that case, we don't want to add the time to additional time paused because we already accounted for it.

src/lib/rtc-to-net/rtc-to-net.ts, line 854 [r23] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> Unnecessary parentheses

Done.

src/lib/rtc-to-net/rtc-to-net.ts, line 855 [r23] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> Can you make this message clearer?

Done.

src/lib/rtc-to-net/bandwidth-config.ts, line 3 [r21] (raw file):
Previously, jpevarnek (Jonathan Pevarnek) wrote…
> In that case, it would be much better to have this be the type we actually user (possibly just exporting it directly from `rtc-to-net`) and use that in the core api.

Done.

Comments from Reviewable

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

src/lib/rtc-to-net/rtc-to-net.ts, line 875 [r26] (raw file):

    public pauseForBandwidthOverflow = (pauseTime: number): void => {
      // reset additional shit

Update this comment :)?

src/lib/rtc-to-net/rtc-to-net.ts, line 878 [r26] (raw file):

      this.additionalTime = 0;
      this.pauseStart = 0;
      this.pauseResume = 0;

Im thinking about a case where I think we might under count the time paused--please let me know if I'm missing something:

Pause for overflow (example: set pauseStart to 10ms)
Pause for bw (set pauseStart to 0ms)
Unpause for bw (pauseStart still 0ms)
Unpause for overflow <-- additional time is not updated because pauseStart is 0ms.

It seems like we wouldn't count the paused time between the first two steps -- does that sound right?

Comments from Reviewable

Review status: 3 of 5 files reviewed at latest revision, 23 unresolved discussions, some commit checks pending.

src/lib/rtc-to-net/rtc-to-net.ts, line 875 [r26] (raw file):
Previously, lucyhe wrote…
> Update this comment :)?

ooomg whoops Done.

src/lib/rtc-to-net/rtc-to-net.ts, line 878 [r26] (raw file):
Previously, lucyhe wrote…
> Im thinking about a case where I think we might under count the time paused--please let me know if I'm missing something:
> - Pause for overflow (example: set pauseStart to 10ms)
> - Pause for bw (set pauseStart to 0ms)
> - Unpause for bw (pauseStart still 0ms)
> - Unpause for overflow <-- additional time is not updated because pauseStart is 0ms.
>
> It seems like we wouldn't count the paused time between the first two steps -- does that sound right?
>

So, the variables you mentioned are updated like that. However, the paused time would be updated. ```pauseForBandwidthOverflow``` is called every interval. Before calling the method though, the original ```calculateBandwidth``` method checks whether there is addtional paused time (was the session paused for socket overflow and has not yet resumed). That time is taken into account, and because this will "pause" it again, the variables are reset.

Comments from Reviewable

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

src/lib/rtc-to-net/rtc-to-net.ts, line 878 [r26] (raw file):
Previously, shilpa10kumar9 wrote…
> So, the variables you mentioned are updated like that. However, the paused time would be updated. ```pauseForBandwidthOverflow``` is called every interval. Before calling the method though, the original ```calculateBandwidth``` method checks whether there is addtional paused time (was the session paused for socket overflow and has not yet resumed). That time is taken into account, and because this will "pause" it again, the variables are reset.

Chatted offline, the only edge case we might miss is actually between steps 3 and 4 above.
Also agreed to move these three set to 0s to calculateBandwidth, in case pause isn't called for an interval

Comments from Reviewable

Review status: 3 of 5 files reviewed at latest revision, 13 unresolved discussions, some commit checks failed.

src/lib/rtc-to-net/rtc-to-net.ts, line 850 [r29] (raw file):

    }

    public calculateBandwidthSession = (): void => {

nit: rename to calculateBandwidth

src/lib/rtc-to-net/rtc-to-net.ts, line 875 [r29] (raw file):

    }

    public pauseSession = (limit: number): void => {

I feel like this should either be named pauseSession and accept time as its parameter, or named setLimit and accept a limit as its parameter.

src/lib/rtc-to-net/rtc-to-net.ts, line 875 [r29] (raw file):

    }

    public pauseSession = (limit: number): void => {

This function now looks a lot like resetForNoPause, I wonder if it makes sense to combine them into a function that accepts a limit, and checks if the bandwidth is over that limit. This one's your call!

src/lib/rtc-to-net/rtc-to-net.ts, line 879 [r29] (raw file):

      var timeToPause = RtcToNet.BANDWIDTH_MONITOR_INTERVAL * (1 - this.notPausedFrac);
      log.debug('%1 is pausing (total experimenting) for %2; total bytes sent/rec: %3', this.channelLabel(), timeToPause, this.currBytes);
      this.pauseStart = 0;

Having read the code a few times I think it would help for these three variables (start, resume and add'l time) to someone capture that they refer to socket overflow. Maybe like socketOverflowPauseStart?

Comments from Reviewable

Review status: 3 of 5 files reviewed at latest revision, 13 unresolved discussions, some commit checks failed.

src/lib/rtc-to-net/rtc-to-net.ts, line 850 [r29] (raw file):
Previously, lucyhe wrote…
> nit: rename to calculateBandwidth

Done.

src/lib/rtc-to-net/rtc-to-net.ts, line 875 [r29] (raw file):
Previously, lucyhe wrote…
> This function now looks a lot like resetForNoPause, I wonder if it makes sense to combine them into a function that accepts a limit, and checks if the bandwidth is over that limit. This one's your call!

It doesn't entirely make sense to combine into one function, because we need to only call pauseSession if the total bw is over the total limit && the session bw is over the seesion limit. Combining into one method would mean passing more variables in to do all those checks, which I think makes it less clear.

src/lib/rtc-to-net/rtc-to-net.ts, line 875 [r29] (raw file):
Previously, lucyhe wrote…
> I feel like this should either be named pauseSession and accept time as its parameter, or named setLimit and accept a limit as its parameter.

I don't think it should be called setLimit bc it already has a limit, but if it goes over the limit, it gets paused for a certain amount of time. I moved a couple of lines back up to RtcToNet in order to make the method accept time as its parameter. Done.

src/lib/rtc-to-net/rtc-to-net.ts, line 879 [r29] (raw file):
Previously, lucyhe wrote…
> Having read the code a few times I think it would help for these three variables (start, resume and add'l time) to someone capture that they refer to socket overflow. Maybe like socketOverflowPauseStart?

Done.

Comments from Reviewable

it looks like this stalled and should be closed, but please reopen if there's more to do here.