Failure to use set_region_of_interest after several stream restarts.

Required Info |
-- | --
Camera Model | { D415 }
Firmware Version | (<IP_ADDRESS>)
Operating System & Version | { Ubuntu 18.04 (NVidia Jetpack 4.4)
Kernel Version (Linux Only) | (4.9)
Platform | NVIDIA Jetson NX (NVidia Jetpack 4.4)
SDK Version | { 2.31 }
Language | {C++ }
Segment | { others }
Issue Description
Sometimes, when we restart several times our pipeline, we have rare happenstance of an error when setting the exposure roi over the rgb stream. Anything we are doing wrong here?
Here's the error we get :
can't set roi { min_x 296, min_y 0, max_x 1659, max_y 643 } on sensor :
hwmon command 0x75( 0 283 128 67b ) failed (response -6= Invalid parameter) (kogmod)
Here's the overall API calls that we do in a single sequence. Anything we are doing wrong here?
 rs2::context ctx_;

 auto devices = ctx_.query_devices();
 const auto count = devices.size();
 if (count == 0)
 {
     throw std::runtime_error("No realsense device found.");
 }

 auto device_restarted{false};
 ctx_.set_devices_changed_callback([&device_restarted](rs2::event_information& info) mutable {
     if (info.get_new_devices().size() != 0)
     {
         device_restarted = true;
     }
 });
 for (auto&& device : devices)
 {
     device.hardware_reset();
 }

auto reset_timeout = std::chrono::steady_clock::now() + chrono::seconds(15);

 do
 {
     if (std::chrono::steady_clock::now() > reset_timeout)
     {
         log_fatal("Failed to wait for device after rs2 hardware_reset.");
     }
     this_thread::sleep_for(chrono::milliseconds(100));
 } while (!device_restarted);

 rs2::pipeline pipe_;
 rs2::config cfg;
 cfg.enable_stream(RS2_STREAM_COLOR, 1920, 1080, RS2_FORMAT_RGB8, 15);
 cfg.enable_stream(RS2_STREAM_DEPTH, 424, 240, RS2_FORMAT_Z16, 15);
 pipe_.start(cfg);

 for (auto&& device : ctx_.query_devices())
 {
     const auto device_name = device.get_info(RS2_CAMERA_INFO_NAME);
     log_trace(string("starting rs2 device : ") + device_name);
     for (auto&& sensor : device.query_sensors())
     {
          //Setting options in this order for the sensors
          // Stereo module
         // RS2_OPTION_EMITTER_ENABLED 1
         // RS2_OPTION_ENABLE_AUTO_EXPOSURE 0
         // RS2_OPTION_ENABLE_AUTO_WHITE_BALANCE 0
         // RS2_OPTION_LASER_POWER 35

         // RGB Camera
         // RS2_OPTION_ENABLE_AUTO_EXPOSURE 1
         // RS2_OPTION_ENABLE_AUTO_WHITE_BALANCE 1
           for (const auto& options : option)
           {

                if (sensor.supports(rs_option->second))
                {
                     sensor.set_option(rs_option->second, item.value().get<float>());
               }
         }
   }
}

for (auto&& device : ctx_.query_devices())
{
  for (auto&& sensor : device.query_sensors())
  {
      if (sensor.is<rs2::roi_sensor>())
      {
          const auto is_rgb = strstr(sensor.get_info(RS2_CAMERA_INFO_NAME), "RGB") != nullptr;
          const auto rs2_exposure = Local::get_exposure(is_rgb);
          try
          {
              sensor.as<rs2::roi_sensor>().set_region_of_interest(rs2_exposure);
          }
          catch(const rs2::error& e)
          {
              std::stringstream ss;
              ss << "can't set roi { " << rs2_exposure.min_x << ", " << rs2_exposure.min_y << ", " << rs2_exposure.max_x << ", " << rs2_exposure.max_y << " } on sensor : " << e.what();
              log_error(ss.str());
           }
      }
 }
}

Hi @jeffeDurand   As a starting point in the investigation, may I ask please if you built librealsense with the RSUSB method?

No, here's the flag I'm passing when configuring with cmake:
mkdir -p build && cd build && cmake -GNinja
-DBUILD_SHARED_LIBS=ON
-DBUILD_EXAMPLES=ON
-DBUILD_UNIT_TESTS=OFF
-DFORCE_RSUSB_BACKEND=OFF
-DBUILD_PYTHON_BINDINGS=OFF
-DBUILD_MATLAB_BINDINGS=OFF
-DBUILD_OPENNI2_BINDINGS=OFF
-DBUILD_NODEJS_BINDINGS=OFF
-DBUILD_CSHARP_BINDINGS=OFF
-DBUILD_UNITY_BINDINGS=OFF
-DBUILD_WITH_TM2=OFF
-DBUILD_WITH_CUDA=OFF
-DBUILD_PYTHON_DOCS=OFF
-DBUILD_NETWORK_DEVICE=OFF
-DCMAKE_CUDA_COMPILER=/usr/local/cuda/bin/nvcc
-DANDROID_USB_HOST_UVC=OFF

Checking through your script, my personal hunch would be to focus on verifying that the sleep timing after reset is correct.  The subject is discussed in detail in the link below
https://github.com/IntelRealSense/librealsense/issues/5349

I would have kind of expect that the set_devices_changed_callback callback would prompt when the device is really ready to fully operate. I guess we'll catch and manage these rare issues has a special case on our end. Thanks.

After some investigating, it appears that the problem happens a lot when no set roi is done for the depth but only for rgb. When I set both, I don't get the same errors.

Hi @jeffeDurand   Is setting both ROI to avoid errors a problem for your project or is it an acceptable solution, please?

What does setting the exposure ROI on the depth stream do?

Hi @logidelic   When a ROI is defined and auto-exposure is enabled, the SDK averages the intensity of the pixels inside the ROI and tries to maintain that intensity at a defined setpoint value.
Setting a separate RGB ROI does not affect the depth image, just the color image.
Point 3 of the section of Intel's camera tuning guide that I have linked to below has further details about defining a ROI in the RealSense Viewer:
https://dev.intelrealsense.com/docs/tuning-depth-cameras-for-best-performance#section-make-sure-image-is-properly-exposed

Thanks for the response Marty. However, I understand what setting an exposure ROI on the RGB stream accomplishes...
My question was specifically with regard to setting this exposure ROI on the depth stream. Does it do anything at all? I'm asking because above you're suggesting that this may be a solution to the problem encountered in the OP...
Thanks again.

@logidelic  The question of the effect that an auto-exposure ROI has was answered above.
When a depth auto-exposure ROI is defined and auto-exposure is enabled, the intensity of the pixels inside the ROI area is averaged and the auto-exposure tries to maintain that intensity at a predefined Setpoint value.
Also, it was @jeffeDurand who stated that setting the depth ROI fixed the problem.

When a depth auto-exposure ROI is defined and auto-exposure is enabled, the intensity of the pixels inside the ROI area is averaged and the auto-exposure tries to maintain that intensity at a predefined Setpoint value.

I guess you are suggesting that this happens for each of the stereo-pair sensors, before they get combined to extrapolate the depth, is that right? Sorry for too many question, just want to be sure I understand.

@logidelic  I am not on the developer team so I do not have that information.  I researched your new question carefully but was not able to locate any further information about how the ROI functions.  I do apologize.

@logidelic It seems that setting the exposure ROI on the "depth" sensor actually affects the exposure of the infrared sensors, as you might expect.

Hmm, that's somewhat interesting, but also a bit hard to understand in terms of how that should be used...


Fix/15131: Special html characters like &nbsp are not escaped in name and can bug out the UI

Details

We are using htmlDecode when displaying the user name in the chat so when users enter  HTML entities like  &nbsp, it's not showing correctly
Fixed Issues

$ https://github.com/Expensify/App/issues/15131
$ PROPOSAL: https://github.com/Expensify/App/issues/15131#issuecomment-1431241018
Tests

Log in to account A and change the user name to &nbsp
Send some messages and send/request money to account B
Verify that the user name of account A is displayed correctly in all the reports, IOU messages and LHN in both accounts

[x] Verify that no errors appear in the JS console

Offline tests

Log in to account A and change the user name to &nbsp
Send some messages and send/request money to another account
Verify that the user name is displayed correctly in all the reports, IOU messages and LHN in account A

QA Steps

Log in to account A and change the user name to &nbsp
Send some messages and send/request money to account B
Verify that the user name of account A is displayed correctly in all the reports, IOU messages and LHN in both accounts

[x] Verify that no errors appear in the JS console

PR Author Checklist

[x] I linked the correct issue in the ### Fixed Issues section above
[x] I wrote clear testing steps that cover the changes made in this PR

[x] I added steps for local testing in the Tests section
[x] I added steps for the expected offline behavior in the Offline steps section
[x] I added steps for Staging and/or Production testing in the QA steps section
[x] I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
[x] I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
[x] I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).

[x] I included screenshots or videos for tests on all platforms
[x] I ran the tests on all platforms & verified they passed on:

[x] Android / native
[x] Android / Chrome
[x] iOS / native
[x] iOS / Safari
[x] MacOS / Chrome / Safari
[x] MacOS / Desktop

[x] I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
[x] I followed proper code patterns (see Reviewing the code)

[x] I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
[x] I verified that comments were added to code that is not self explanatory
[x] I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
[x] I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method

[x] If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:

[x] I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
[x] I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
[x] I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
[x] I verified the JSDocs style guidelines (in STYLE.md) were followed

[x] If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
[x] I followed the guidelines as stated in the Review Guidelines
[x] I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
[x] I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
[x] I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
[x] I verified that if a function's arguments changed that all usages have also been updated correctly
[x] If a new component is created I verified that:

[x] A similar component doesn't exist in the codebase
[x] All props are defined accurately and each prop has a /** comment above it */
[x] The file is named correctly
[x] The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
[x] The only data being stored in the state is data necessary for rendering and nothing else
[x] For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
[x] Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
[x] All JSX used for rendering exists in the render method
[x] The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions

[x] If any new file was added I verified that:

[x] The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory

[x] If a new CSS style is added I verified that:

[x] A similar style doesn't already exist
[x] The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)

[x] If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
[x] If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
[x] If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
[x] If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
[x] I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web
https://user-images.githubusercontent.com/113963320/222045910-76131a8e-e4a5-4772-aa30-8aed5492ab67.mp4

Mobile Web - Chrome
https://user-images.githubusercontent.com/113963320/222045958-95552738-7cc1-4ba4-aab7-749309e701cf.mp4

Mobile Web - Safari
https://user-images.githubusercontent.com/113963320/222046321-bb81b9de-c437-4fe2-a1db-628da8a7df61.mp4

Desktop
https://user-images.githubusercontent.com/113963320/222046042-d47f8c9d-108e-4e14-8924-ec7de3a0e687.mp4

iOS
https://user-images.githubusercontent.com/113963320/222046076-cf2143a9-cc86-4a1e-815b-b8f68c2c93b5.mp4

Android
https://user-images.githubusercontent.com/113963320/222047968-259257aa-5d5d-40e8-a1e0-449a442573f0.mp4

@pecanoro @sobitneupane One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

@pecanoro should we be CP-ing this to Staging?

@tienifr Can you please retest and update screenshots in PR body? I am not getting expected results at all.

@tienifr Can you please retest and update screenshots in PR body? I am not getting expected results at all.

@sobitneupane Sure, I am working on it

@tienifr Looks like the PR at the backend is in staging(https://staging.new.expensify.com/). So, changes are not yet reflected on production(https://new.expensify.com/).
We might have to wait for the PR at the backend to hit production before testing.

@sobitneupane We can update the backend URL in the code to the URL of the staging server to test

@sobitneupane We can update the backend URL in the code to the URL of the staging server to test

I think we are having some issues while updating the backend URL in the code: https://expensify.slack.com/archives/C01GTK53T8Q/p1677569083684259. If it works for you, you can test it and let me know once done.

@sobitneupane I just update the body of this PR. About updating the URL of the staging server to test:

For Web: Update the host in proxy like below

For Mobile Apps and Desktop: Update the expensifyURL in the CONFIG.js like below

@tienifr
This is the mail I received on request money. I was expecting "Requested NPR 1.00 from &#38 sobit" but I got "Requested NPR 1.00 from &#38 sobit"

@tienifr Can you please test Push Notifications as well? I am not sure how can I test it on native.

@sobitneupane Thanks for your review. I think sending mail notification is the work from BE side, and in order to test push notification on native device, we have to build app on physical device. Please correct me if I'm wrong.

Unfortunately, the physical devices are not available for now, and It can take times for testing. Can you help test push notification first, and I'll post the screenshots later on. Sorry for this inconvenience

@tienifr You might have missed https://github.com/Expensify/App/pull/15340#discussion_r1121509557.

in order to test push notification on native device, we have to build app on physical device.

I tried testing it on physical device but was unable to get any push notifications.

@sobitneupane I'll update as your comment asap.
About testing push notification on native, did you try this https://app.slack.com/client/T03SC9DTT/C01GTK53T8Q?

About testing push notification on native, did you try this https://app.slack.com/client/T03SC9DTT/C01GTK53T8Q?

The attached link does not open.

@sobitneupane Can i try this link https://expensify.slack.com/archives/C01GTK53T8Q/p1675092451431619?

Push Notifications are not decoded. The issue exist even in simple messages like "hello &#38" which is not related to this PR. But IOU notification issue must be caused by the change in backend to encode IOU Action.

I tested on branch main with production BE (not staging) and the issue also happened but only with some specific currencies include "NPR" (picture 2,3,4). So I don't think the problem comes from the change in BE for this PR.

I think change in backend has already deployed to production. So, it is reproducible with production api as well.

There is a larger discussion about this in another issue, the problem right now is happening for all messages (I think) as we are encoding the text and not only the HTML. Either way, I think the message is going to be fixed in https://github.com/Expensify/App/issues/15026, so maybe we should just stick to the profile names?

As @sobitneupane mentioned in this comment https://github.com/Expensify/App/pull/15340#pullrequestreview-1319582677,  profile name is well displayed.He also tested with the push notification https://github.com/Expensify/App/pull/15340#issuecomment-1451594467 and it looks good. Personally I think that the issue with profile name is addressed in my PR. What are your thoughts @pecanoro? Please let me know if there is anything else we have to address. Thank you!
@sobitneupane I did resolve your comment . Please help me check it. Thank you!

@sobitneupane Can you restest everything and make sure it works fine after the changes? Then I can merge

@sobitneupane Can you help me retest everything since I had updated the code?

"hello &#38" is shown as "hello &" in LHN. It is reproducible in staging as well. I remember it working fine when I tested it last time. @tienifr Is it a regression from any other PR?

@sobitneupane I think there are some changes in BE that they don't encode the message anymore. The decode logic in FE remains therefore you encountered the issues.
@pecanoro said that the PR should stick to the display name. If there is any problem with the message, we should create another issue.
In case you want to handle the issue in LHN, we can remove decode logic like we used to https://github.com/Expensify/App/pull/15340#discussion_r1114575316

@pecanoro This issue is not directly related to this PR. Do we want to handle it here or create a separate issue for it?

@sobitneupane https://github.com/Expensify/App/pull/15340#discussion_r1119057575 as you mentioned, the message is used to be encoded but I have just checked and found that it is not being encoded now.

@tienifr So, does that mean normal messages are not encoded any longer but IOUQuote are encoded?

does that mean normal messages are not encoded any longer but IOUQuote are encoded?

Yes, it happened in the BE side.

The message itself in chat doesn't seem related to this and there is an issue for this here: https://github.com/Expensify/App/issues/15508
However, I am wondering if we should fix the IOU on the LHN? Is it because we are also using text instead of HTML?

Oh ok, so this is coming from somewhere else since it's already being decoded. So we probably need to encode it in the BE, right? If so, I think we are good.

@pecanoro you are correct. We need to encode it in the BE (we used to do that). It's the best approach since we don't want to mess the FE up.
This comment shows that the LHN was good until some changes happened in the BE.

@sobitneupane all good then?

@pecanoro Since we don't encode data (IOU message, normal message) in the BE, we can consider keeping the FE as it is in the main branch right now and only removing redundant HTML decode in the FE:
https://github.com/Expensify/App/blob/657b439007004a5a128fc76df00327cb14203915/src/components/ReportActionItem/IOUQuote.js#L86
https://github.com/Expensify/App/blob/657b439007004a5a128fc76df00327cb14203915/src/libs/Notification/LocalNotification/BrowserNotifications.js#L110-L113
https://github.com/Expensify/App/blob/657b439007004a5a128fc76df00327cb14203915/src/pages/home/report/ReportActionItemFragment.js#L145
https://github.com/Expensify/App/blob/657b439007004a5a128fc76df00327cb14203915/src/libs/SidebarUtils.js#L249
It works in both web and mobile.
https://user-images.githubusercontent.com/113963320/224206238-378af5a1-32bf-4196-b7cc-62be6ce63ad3.mp4
https://user-images.githubusercontent.com/113963320/224206247-b3c6f99e-d6a6-4a89-a95a-1ed1db1af30d.mp4
A cons of this solution is it may be inconsistent with other PRs or fixes because in this PR they are still using htmlDecode to decode html.

I actually like your idea until we decide how to make comments and IOUs consistent in the back end.

Are we going to create a thread in Slack about how we keep comments and IOUs consistent in the back end?

@sobitneupane Did you check @tienifr message here? It seems a better approach until we get some consistent back-end behavior.

@tienifr I like the idea. But I think it will require some backend changes.

we don't encode data (IOU message, normal message) in the BE

After your request here, I believe we do encode IOU message in the backend.
Result after removing Str.htmlDecode.

cc: @pecanoro

@sobitneupane the issue with "&nbsp" after currency can not prove that we do encode IOU message in the backend. Moreover, if we do encode and we remove Str.htmlDecode, "&#38" should become "&#38". You can test on the main branch to verify.

@tienifr I like the https://github.com/Expensify/App/pull/15340#issuecomment-1463119526. But I think it will require some backend changes.
I think my new idea is compatible with the current BE because I can prove that they are not doing encode now.

Hi @sobitneupane, do we have any problem keeping my current code? I'm quite confused because we do decode in IOU message in report but we might need to remove decode in LHN to handle the remain issue?

@sobitneupane I think my new idea is compatible with the current BE and we don't need any changes.

@sobitneupane I think my new idea is compatible with the current BE and we don't need any changes in BE.

If you have tested your current idea with NPR and didn't see the issue I mentioned in https://github.com/Expensify/App/pull/15340#issuecomment-1464922392, you can go ahead with your new idea.

@sobitneupane

If you have tested your current idea with NPR and didn't see the issue I mentioned in https://github.com/Expensify/App/pull/15340#issuecomment-1464922392, you can go ahead with your new idea.

The problem is they encode the whitespace after NPR but they don't encode the name &#38 and the comment opt &#38 in "text" responded from BE. If they encode the name &#38 and the comment opt &#38, it should become
&#38 and opt &#38 (different from your screenshot).
I have tested and found that when we send a IOU request and the browser show a notification:
Expected behavior

Request NPR 100.00 from &#38 for &nbsp

Actual result

Removing Str.htmlEncode in https://github.com/Expensify/App/blob/57a2129b9288cbbee339514822a5dd64e73b6804/src/libs/Notification/LocalNotification/BrowserNotifications.js#L113

Keeping Str.htmlEncode in https://github.com/Expensify/App/blob/57a2129b9288cbbee339514822a5dd64e73b6804/src/libs/Notification/LocalNotification/BrowserNotifications.js#L113

To summarize our solutions:

Solution 1: If we want to keep the decoding logic, we need to use  "html" in message responded from BE instead of "text" to render UI in every place.
Solution 2: If we don't want to keep the decoding logic, we need to remove it and we can directly use "text" in message responded from BE.

@pecanoro Looks like we are only encoding a part of the IOU message in the backend and returning "text". Can we remove all encoding of IOU message in "text"

@pecanoro The remain problem is we are encoding only a part of the IOU message in the backend -> inconsistent behavior. Therefore, we can not handle all the issues. I proposed 2 solutions here, please check it and let us know your thoughts. Thank you!

When I applied the back-end changes, I applied it to the whole thing, but let me double check if it changed or we are encoding twice.

@pecanoro Can you help me check which form of the message is encoded and returned? "text" or "html"? Thank you!

I think we want to fix this throughout the app? I see some places where this has not been handled e.g. SidebarUtils and OptionsListUtils. I have read some comments, just here to ask to not decode plaintext. i.e.:
lastMessageTextFromReport = Str.htmlDecode(report ? report.lastMessageText : '');

should be
lastMessageTextFromReport = report ? report.lastMessageText : '';

If there was regression in those, they should have been fixed by now, since the backend now properly escape html entities constructing text from html.

I thought the backend fix included IOUQuote. I think it's better to address this to be fixed in backend if it's not much work. Also I think using htmlDecode alone is not enough, we need to strip html tags as well.

We are definitely encoding both the text and the HTML in the back end. Let me fix that and we can decide what to use. Though I am going to stop encoding text for every message, not only IOU, so I am worried I might break something.

While I fix the above, why can't we use the HTML part instead?

Either way, I am going to start using htmlspecialchars (docs) on the back-end, meaning I will encode all special characters.

For the html part, we need to mimic the backend functionality of converting that html to text which is done in two steps (I think):

Strip html tags
Decode message

By looking at the code I think only step 2 is done and step 1 is skipped, this may introduce a regression if the message is <strong>hello <</strong>:

Expected: hello <
Using htmlDecode only: <strong>hello <</strong>

@pecanoro Why you are going to use htmlspecialchars, or the real question on what you are going to use that on?

On html? This will cause a regression as we already send messages encoded but there may be an edge case where the IOU messages are not encoded
On text? I don't think you meant this, just asking to be sure. For text we want to decode the special chars not encode them.

Ok, I think I figured out what's missing here. For some reason, we strip out and encode all HTML entities of the IOU comment and they get lost, they are not even in the database. I am going to try to fix that and I think that will solve a lot of problems with this PR.

About the IOUQuote. I see it's being sent completely, but the HTML entities get lost somewhere in the way and it's not being sent with the entities encoded as we do with the normal message.

Ok, I think we need to encode the IOUQuote as well or HTML tags will get lost as we do that for security reasons. I will fix that tomorrow and then I think all our problems will be solved.

I have a PR here to fix the problems with sending the IOU comments to the back-end. It seems to be decoded properly on the chat but not in the details, do you want to fix the decoding problem in this PR or should I fix it in the other one?

Nvm, forget what I said in my last comment, we needed a PR in the back end as well to sent the text messages properly.

Since I ended up fixing the messages here, let's just focus on the display names here so we don't have to put a HOLD on this PR.

@tienifr Let's resolve https://github.com/Expensify/App/pull/16055#issuecomment-1477882933 issue as well in this PR.

@sobitneupane  @pecanoro After my update:

display name + IOU message in chat: correct

display name + IOU message in LHN: correct

display name + IOU message in Details: correct

display name + IOU message in BrowserNotifications: correct

Hi @pecanoro, please help check my lastest comment. Thank you!

Hi @sobitneupane @pecanoro I resolved the conflict and updated the code. Please help me review. Thank you!

@mountiny I just updated
cc @sobitneupane @pecanoro

Sorry guys, I was on vacation for a few days, I will final review and test this today.

:hand: This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

🚀 Deployed to staging by https://github.com/pecanoro in version: 1.2.91-0 🚀

platform
result

🤖 android 🤖
success ✅

🖥 desktop 🖥
success ✅

🍎 iOS 🍎
success ✅

🕸 web 🕸
success ✅

🚀 Deployed to production by https://github.com/luacmartins in version: 1.2.91-1 🚀

platform
result

🤖 android 🤖
success ✅

🖥 desktop 🖥
success ✅

🍎 iOS 🍎
success ✅

🕸 web 🕸
success ✅


Multi-injection support

Hey guys! I am heavily using this library in one of my projects, and just ran into a good use case for multi-injection. More specifically, I want to to something like (pseudo code):
  // Provide the implementations, holding them all inside the container
  Container.bind(MyAbstractClass).add(MyConcreteClass1);
  Container.bind(MyAbstractClass).add(MyConcreteClass2);

   // Inject all registered implementations
   constructor(
      @MultiInject private _allImplementations: MyAbstractClass[]
  ) { }

Just to make sure, is there some way to achieve this today that I am missing?
If not, do you guys think it would be a good feature to add?

Let me know your thoughts!

Hi @julianosam ,
It is not possible with the current version, but I like the idea. We can think something to next release

I had the same requirement and found a workaround:
// components/base.ts
export abstract class BaseClass {
}

// components/impl1.ts
import { Provides } from "typescript-ioc";
import { BaseClass } from "./base";

@Provides(BaseClass)
export class ImplClass1 extends BaseClass {
}

// components/impl2.ts
import { Provides } from "typescript-ioc";
import { BaseClass } from "./base";

@Provides(BaseClass)
export class ImplClass2 extends BaseClass {
}

// components/index.ts
import { Container } from "typescript-ioc";
import { BaseClass } from "./base";
import { ImplClass1 } from "./impl1";
import { ImplClass2 } from "./impl2";

// this is where the magic happens
export class ImplCollection extends Array<BaseClass> {
  constructor() {
    super();
    this.push(Container.get(ImplClass1) as BaseClass);
    this.push(Container.get(ImplClass2) as BaseClass);
  }
}

// main.ts
import { AutoWired, Inject } from "typescript-ioc";
import { ImplCollection } from "./components/index";

@AutoWired
class Consumer {
  @Inject
  private implementations: ImplCollection;
}

let consumer = new Consumer();

Any updates on this?
While the given workaround will achieve the required behavior I think it should work out of the box for a dependency injection container. I have yet to work with an ioc container that doesn't support this feature.
This usually comes with a "named" injection - i.e something like the following:
@Name('A')
@Provies(BaseClass)
export class A extend BaseClass { ... }

Then you can do something like
export class B {
    @inject('A')
    public _myBaseBlassWhichIsA: BaseClass;
}

Below is an example (described in a test) of the unity container behavior with names and multiple implementations:
http://www.tomdupont.net/2014/02/understanding-unity-named-registration.html

any intentions to implement this?

Aw, just ran into this.

Aw, just ran into this.


Split forge-runner/lib.rs

Split this file into smaller, more contained modules.

Blocked by test collector


[BUG] @snowpack/plugin-webpack not working when custom root is set

Bug Report Quick Checklist

[x] I am on the latest version of Snowpack & all plugins.
[x] I use package manager - yarn
[x] I run Snowpack on OS Mac.
[x] I run Snowpack on Node.js v12+

Describe the bug
When custom root is set in snowpack.config.js file, the snowpack webpack plugin is not working.
snowpack config:
module.exports = {
  root: "myapp",
  buildOptions: {
    out: "dist"
  },
  plugins: [["@snowpack/plugin-webpack"]]
};

Build output:
sandbox@sse-sandbox-uvqrk:/sandbox$ yarn build
yarn run v1.22.10
warning package.json: No license field
$ snowpack build
[12:36:13] [snowpack] ! building files...
[12:36:13] [snowpack] ✔ files built. [0.01s]
[12:36:13] [snowpack] ! building dependencies...
[12:36:13] [snowpack] ✔ dependencies built. [0.00s]
[12:36:13] [snowpack] ! writing to disk...
[12:36:13] [snowpack] ✔ write complete. [0.00s]
[12:36:13] [snowpack] ! optimizing build...
[12:36:13] [@snowpack/plugin-webpack] Error: ENOENT: no such file or directory, open '/sandbox/myapp/package.json'
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

To Reproduce
We can't fix bugs that we can't see for ourselves. Issues often need to be closed if this section is skipped.

The issue can be reproduced in this codesandbox -> https://codesandbox.io/s/snowpack-webpack-root-issue-uvqrk?file=/snowpack.config.js
Create a terminal & run yarn build
See error!

Expected behavior
Webpack should be used to bundle the assets

The docs don't give a lot of detail for root, but it's meant to be the root of the whole project,  i.e. where your config files are, package.json etc. If that is the case, the Webpack plugin is doing what it should: it's looking in package.json for a browserlist config so it can pass that on to @babel/preset-env.
In your case, it looks like what you're trying to specify with root is where to find index.html. If so, try this:
// snowpack.config.js
module.exports = {
  mount: {
    myapp: '/',
    src: '/src',
  },
  buildOptions: {
    out: "dist"
  },
  plugins: [["@snowpack/plugin-webpack"]]
};

Then, just a quick change to index.html - the script path is relative to where you tell Snowpack to mount everything, not your local file structure:
// myapp/index.html
...
<script type="module" src="/src/general.js"></script>
...

This works for me (the alert is triggered) both in snowpack dev or if I serve the dist folder locally.

Works like a charm. Thanks @jalovatt


Separate storage by network

good point

Fixed by @alexvandesande here https://github.com/ethereum/ens-registrar-dapp/pull/42