adding carousel to attachment modal

Details

add a carousel to the attachment modal that navigates through a chat's attachments
Fixed Issues

$ https://github.com/Expensify/App/issues/7862
PROPOSAL: https://github.com/Expensify/App/issues/7862#issuecomment-1128202430
Tests

iOS / android

tap on an image in chat and an attachment modal presents
swipe left or right to change the attachment to the next or previous one
tap on the modal to make the arrows on the sides appear or disappear
tap on an arrow to change the attachment to the next or previous one
verify attachment's name are displayed correctly in modal header

web / desktop

click an image
clicking an arrow will change the attachment to the next or previous one
verify that moving the cursor outside the modal will make the arrows disappear
verify moving the cursor back into the modal will make the arrows appear
verify modal header matches file name, and correct attachment is downloaded

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

Offline tests

QA Steps

iOS / android

tap on an image in chat and an attachment modal presents
swipe left or right to change the attachment to the next or previous one
tap on the modal to make the arrows on the sides appear or disappear
tap on an arrow to change the attachment to the next or previous one
verify attachment's name are displayed correctly in modal header

web / desktop

click an image
clicking an arrow will change the attachment to the next or previous one
verify that moving the cursor outside the modal will make the arrows disappear
verify moving the cursor back into the modal will make the arrows appear
verify modal header matches file name, and correct attachment is downloaded

[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] 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] 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/6119181/209240380-e1d0d2c8-6d9b-40a9-a13f-d73cd30bf659.mov

Mobile Web - Chrome

https://user-images.githubusercontent.com/6119181/209581657-961beb4a-41c9-40f1-9435-491643434669.mov

Mobile Web - Safari

https://user-images.githubusercontent.com/6119181/209581646-c0f64f3b-81fb-497e-9976-0a6e603a92e9.mov

Desktop

https://user-images.githubusercontent.com/6119181/209240439-beb8e0c8-a46f-418b-acb4-d56e82bf65e4.mov

iOS

https://user-images.githubusercontent.com/6119181/209240861-68fc6624-4096-4186-936b-4291fcfca9b4.mov

Android
https://user-images.githubusercontent.com/6119181/210114328-cbd4e055-382f-4710-92fd-3fb50e3831e7.MOV

Review paused until santhosh get's back from OOO. Adding him as reviewer so that he sees the PR once he's back.

@JediWattson From issue description https://github.com/Expensify/App/issues/7862#issue-1146471345
Expected Result:

When the member is viewing the most recent attachment in the chat, there's only a left arrow icon on the attachment preview screen to navigate further back. Similarly, when the member is viewing the oldest attachment in the chat, there's only a right arrow to navigate forward.
If the member is previewing an attachment that isn't the oldest or latest in the chat history, there will be both left/right arrow icon to navigate "forward" or "back" to the next attachment preview.
On desktop/Web: The arrow can be clicked to navigate to the next attachment preview, or the left/right arrow keys on the keyboard can be used
On mobile/mWeb: The arrow can be tapped or the member can swipe left/right on the preview to navigate to the next attachment preview depending on the direction
On desktop/Web, the arrows are visible on hover. On mobile/mWeb, a single tap on the image hides/shows the arrows.

I believe PR covers only 1 & 2 what above 3, 4, 5.

@JediWattson From issue description #7862 (comment)
Expected Result:

When the member is viewing the most recent attachment in the chat, there's only a left arrow icon on the attachment preview screen to navigate further back. Similarly, when the member is viewing the oldest attachment in the chat, there's only a right arrow to navigate forward.
If the member is previewing an attachment that isn't the oldest or latest in the chat history, there will be both left/right arrow icon to navigate "forward" or "back" to the next attachment preview.
On desktop/Web: The arrow can be clicked to navigate to the next attachment preview, or the left/right arrow keys on the keyboard can be used
On mobile/mWeb: The arrow can be tapped or the member can swipe left/right on the preview to navigate to the next attachment preview depending on the direction
On desktop/Web, the arrows are visible on hover. On mobile/mWeb, a single tap on the image hides/shows the arrows.

I believe PR covers only 1 & 2 what above 3, 4, 5.

for 3 I would propose to add a listener to handle the arrow press. It would be added when the component is mounted:
   componentDidMount() {
        document.addEventListener('keydown', this.handleArrowPress)
    }

    componentWillUnmount() {
        document.removeEventListener('keydown', this.handleArrowPress)
    }

handleArrowPress will check the e.key and call cycleThroughAttachments
    /**
     * Listens for keyboard shortcuts and applies the action
     *
     * @param {Object} e
     */
    handleArrowPress(e) {
        if(e.key === "ArrowLeft" && !this.state.isBackDisabled) {
            this.cycleThroughAttachments(-1);
        }
        if(e.key === "ArrowRight" && !this.state.isForwardDisabled) {
            this.cycleThroughAttachments(1);
        }
    }

I see a SwipeableView but I'm unsure how I want to handle the data change because I feel it's best like this:

  <SwipeableView>
    <AttachmentView sourceURL={sourceURL} file={this.state.file} />
  </SwipeableView>
  // ... <AttachmentCarousel

On mobile you want the arrows to initially appear then disappear onPress right? For desktop they would be visible onMouseOver?

@JediWattson Sounds good! Please make the suggested changes, let me know when changes are done.

For mobile, I think it's okay if they are visible by default and then tapping the image hides them.

I just got most of the changes here up. I just need to add the functionality for the arrows to show and not show

I'm finishing up the arrows today, and then possibly get into the swiping too. I can add new videos showing this this weekend too.

@JediWattson You should use only one git account. Do not use multiple accounts. Can you revert & commit changes properly thanks!

Will do, I think I've completed the requirements now too. Now it seems I just need to clean that up, and add some videos for swiping and toggling arrows

I believe all I have left now is to make some videos showing the arrows show and not showing along with swiping and update the qa instructions

Good then,

Pull changes from the latest main
Update PR Description & videos

And let us know when PR is ready for review.

@JediWattson Bump!

@Santhosh-Sellavel I got some new videos up to show the swiping in native apps. A couple things I really can't figure out are: in mobile browsers I'm unable to capture an event to toggle showing the arrows, and in a pdf I couldn't get a similar event so I left the arrows on there

A couple things I really can't figure out are: in mobile browsers I'm unable to capture an event to toggle showing the arrows, and in a pdf I couldn't get a similar event so I left the arrows on there

Sorry for the delay, are these both specific to mobile web alone?

@JediWattson bump!

the arrows not toggling is specific to mobile browsers, but the pdf seems to be an issue for both mobile browsers and native

I believe I got this 100% now! @Santhosh-Sellavel

Good @JediWattson KIndly update your PR Description along with all other necessary info and updated screen recordings too thanks!

@JediWattson Bump!

@Santhosh-Sellavel apologies, I just need to update the mobile web videos to show the arrows showing/disappearing and it should be good. I'll try to finish that up in the next couple days

@JediWattson bump!

@Santhosh-Sellavel I have updated the mobile web videos to reflect the new changes. I should 100% hopefully!

Please resolve conflicts & let us know when it is ready for review again thanks!
@JediWattson

@Santhosh-Sellavel  conflicts are resolved!

What about the Other changes I requested in the review, please address that too..

@Santhosh-Sellavel do you mean the attachments not working? That should be fixed now

Can you check other review comments

@JediWattson
Sorry for the delay here, I'm not sure why you end up modifying the Swipeable View for onPress.
Adding more swipe-related functionality is fine but not okay with adding irrelevant interaction here.
Before moving forward we need to know,
What other options did you explore i.e without modifying this Swipeable component?
What's the problem with that method?
Why did you think this is the best way to address this?
If you summarise this with enough info, we will get to know more context on this to help us decide on it. We can also take this to slack to get other help on problems you are facing.
Thanks!
cc: @chiragsalian

What other options did you explore i.e without modifying this Swipeable component?
I tried to putting Pressable in a place that would take the onTap gesture but not capture any other gestures that make swiping possible.
For native mobile I couldn't find a place where that was possible so I added that in Swipeable.
What's the problem with that method?
It doesn't work for mobile browsers and moving Pressable out of there obstructs using Swipeable in native mobile
Why did you think this is the best way to address this?
because it's a use case for mobile web where I can't handle onClick or something that's distinct from mobile and not have it hinder the functionality of mobile.

@JediWattson Can you elaborate bit in details, please include the snippets/commits which we can use to play around with.

If I did something like in the below code I'm unable to capture the gestures in Swipable because pressable takes the events now, however it works in mobile web.
<Pressable>
  <Swipable />
</Pressable>

so I used Swipable to handle the tap gesture on native mobile, and then with mobile web the Swipable/index.js containing Pressable now works for web as well

I don't get the point of splitting up Swipable if the web based component isn't used to mirror the events found in the native component? I mean why have the index.native.js when that's what it seems to be there for?

I don't get the point of splitting up Swipable if the web based component isn't used to mirror the events found in the native component? I mean why have the index.native.js when that's what it seems to be there for?

I'm not getting it, can you ask in other words?

why is this component split into two variants for native and web if not to be modified so as to mirror interactions amongst the platforms?

why is this component split into two variants for native and web

There is a comment there already, we are supporting swipe only for native apps it seems!
https://github.com/Expensify/App/blob/cc31727073a1f6dfdec49af47689064b598d20bd/src/components/SwipeableView/index.js#L1-L3

but now that I needed to add a tap gesture in the SwipeableView that's used for both mobile native and web platforms then it makes sense to modify that to reduce code reuse

Swipeable is intended for swipe only actions.
Pressable is intended for tap actions
I don't get it why we want to add a press to Swipeable.
If we need to combine both swipe and tap we should form a new component.
Example take a look at PressableWithSecondaryInteraction which combines longpress & click

can I rename Swipable to SwipableWithPressable then? That makes sense, and that comment explains why swiping is only available for iOS/Android

@JediWattson
Let's leave Swipeable untouched.
SwipeableWithPressable is not need to be defined we might not use it for other
Sometimes it's okay to reuse code.
Actually, as I said it's better to write a new component Attachment Carousel Action Listener.
to define touch/pan/tap listeners split platform-specific only if required.
Props of the component be toggleArrows, nextAttachment or previousAttachment etc.

is there an example of this somewhere else in the app so I can understand better what the ask is here?
also SwipeableWithPressable is just the renaming of  Swipeable and would solve what you're saying here because it's identifying this component is now pressable too. I mean currently Swipeable is technically incorrect with that pressable gesture being handled now.

@chiragsalian or @marcaaron do either of you have an opinion on this https://github.com/Expensify/App/pull/9279#issuecomment-1257185660 so we can help keep this moving and get across the finish line?

I'm not really up to speed on this conversation. Can someone provided a quick summary of what they need help with?

Discussing some of the pending points,

also SwipeableWithPressable is just the renaming of Swipeable and would solve what you're saying here because it's identifying this component is now pressable too.

and,

can I rename Swipable to SwipableWithPressable then? That makes sense, and that comment explains why swiping is only available for iOS/Android

At this point I think it's better to write multiple Attachment Carousel Action Listener, each having platform specific code. This way one can use swipeable for mobile and another can use pressable for web. I think this is what Santhosh was suggesting too, correct me if I'm wrong. Plus I still don't get why pressable is in swipeable. I don't quite understand why using panresponder means we have to have pressable in swipeable. Maybe if you can explain this more?

is there an example of this somewhere else in the app so I can understand better what the ask is here?

Sure does this help, basically, if you look at any of our index.native.js files you can see how we write platform-specific codes. Eg: web, mobile.

@marcaaron
PR is not reviewed completely yet this discussion occurred from the initial review itself.
The interaction requirements for this issue,

On desktop/Web: The arrow can be clicked to navigate to the next attachment preview, or the left/right arrow keys on the keyboard can be used
On mobile/mWeb: The arrow can be tapped or the member can swipe left/right on the preview to navigate to the next attachment preview depending on the direction
On desktop/Web, the arrows are visible on hover. On mobile/mWeb, a single tap on the image hides/shows the arrows.

To achieve this we need to listen,

Swipe events on native & mWeb platforms (to navigate between view next and previous attachment)
Tap event on all platforms (to toggle of prev & next arrows)
hover event on web/desktop(to show arrows)

Now what @JediWattson had done is modified the SwipeableView, to add more swipe functionalities currently, we have only swipeDown action there, also for SwibeableView is available only for native platforms.

It's okay to add more swipe actions like swipeHorizontal
But adding onPress to SwipeableView doesn't make any sense,  SwipeableView implementation of web/desktop has Pressable wrap.

So I requested changes to modify it in a way, that keeps swipe-related & pressable interactions separate.
@JediWattson says they have a problem keeping it separate as some interactions don't work. Didn't get a clear idea on why even after follow-ups.
So I suggested two options,

Write a new component that listens to both swipe & pressable, like SwipeableWithPressable to listen to both events
Then I felt since we do this specifically for this issue, it would be better to write a separate component Attachment Carousel Action Listener and define touch/pan/tap listeners and add props functionalities of the component be like toggleArrows, nextAttachment or previousAttachment etc.

But so far @JediWattson suggesting is renaming SwipeableView to SwipeableWithPressable or modifying SwipeableView
cc: @chiragsalian

Now what @JediWattson had done is modified the SwipeableView, to add more swipe functionalities currently, we have only swipeDown action there, also for SwibeableView is available only for native platforms.

Ah hmm thanks for the explanation @Santhosh-Sellavel @chiragsalian. I agree and think we are trying to do too much by expanding the SwipeableView to "swipe in other directions". IMO it would be easiest to just make a nice contained Carousel component and focus on getting it working without trying to make things 100% reusable. The SwipeableView is a fairly simple component IIRC so I agree with the simpler approach laid out here.

I think I understand the requirements here. For SwipeableView should I revert that code back to its original state then?

Yes

@Santhosh-Sellavel this should be ready to get looked at again!

Please resolve conflicts!

done!

Can you resolve conflicts

@JediWattson please resolve the conflicts and request for review so I can check this one.
You can find the request for review button here

@Santhosh-Sellavel should be all set!!

Friendly bump @Santhosh-Sellavel to review when you get the chance.

Yeah, I'm super excited to finally get this one over the line... doubly so before we run into more conflicts! 😅

Friendly bump @Santhosh-Sellavel to also review, and any updates for us @JediWattson?

I’m going to try and take care of these tomorrow

Is this ready for review or still under work?
I'll assume its ready for review this time. Next time @JediWattson could you leave a message like "ready for review" or click the Request review (button next to the reviewers on the top left),

so that we know it's ready to review and don't accidentally review it while you are working on the code.

@Santhosh-Sellavel are those the latest changes? I fixed something in my last commit that prevents the arrow from working when it's at the end. I'm having trouble disabling the arrow button though. I'll keep working on it

@JediWattson Is this ready for review?

Yup!!
On Mon, Dec 5, 2022 at 1:24 PM Santhoshkumar Sellavel <
@.***> wrote:

@JediWattson https://github.com/JediWattson Is this ready for review?
[image: Screenshot 2022-12-05 at 11 54 15 PM]
https://user-images.githubusercontent.com/85645967/205713276-a575bd72-9c4c-450d-abf9-5cb3b49c311c.png
—
Reply to this email directly, view it on GitHub
https://github.com/Expensify/App/pull/9279#issuecomment-1337902290, or
unsubscribe
https://github.com/notifications/unsubscribe-auth/ABOV6DJ6GZOLCLC33NCD6HLWLYXOZANCNFSM5XSO6BTA
.
You are receiving this because you were mentioned.Message ID:
@.***>

@JediWattson Still this exists https://github.com/Expensify/App/pull/9279#pullrequestreview-1188769102. Can you please fix this?
https://user-images.githubusercontent.com/85645967/205991650-ade186e6-4245-4cd4-b421-6d3c06f54c88.mov

@Santhosh-Sellavel could I see some logs on that error?

@JediWattson I'm not getting any error relevant error logs

@Santhosh-Sellavel I just loaded up a bunch of images and got it to work for me. I'm wondering if there's some bad data in your chat, but that's still something I'll need to look for
https://user-images.githubusercontent.com/6119181/206023415-81c4f4fd-376e-4bf8-a4e6-12e13eb20f39.mov

@JediWattson
This is what I'm getting.
https://user-images.githubusercontent.com/85645967/206025204-00db0a46-0290-47a1-8d2a-f3f826a07b80.mov
@chiragsalian Can you do quick testing?

@Santhosh-Sellavel I really wish I could see the url that's being set for this

@JediWattson
Here are the account details,
Email<EMAIL_ADDRESS>Password: 1TestAccount
report-url = http://localhost:8080/r/78056073

@Santhosh-Sellavel thank you!!! That helped me figure out what you were seeing (and gave me some insight into this app's storage lol)
It's ready to test again and should be working as expected in this case!!!

Attachment download issue after cycling
https://user-images.githubusercontent.com/85645967/206261723-a787d552-0297-40e4-a589-1b40ef805278.mov

[ ] When the member is viewing the most recent attachment in the chat, there's only a left arrow icon on the attachment preview screen to navigate further back. Similarly, when the member is viewing the oldest attachment in the chat, there's only a right arrow to navigate forward.
[x] If the member is previewing an attachment that isn't the oldest or latest in the chat history, there will be both left/right arrow icon to navigate "forward" or "back" to the next attachment preview.
[x] On desktop/Web: The arrow can be clicked to navigate to the next attachment preview, or the left/right arrow keys on the keyboard can be used
[ ] On mobile/mWeb: The arrow can be tapped or the member can swipe left/right on the preview to navigate to the next attachment preview depending on the direction
[x] On desktop/Web, the arrows are visible on hover.
[ ] On mobile/mWeb, a single tap on the image hides/shows the arrows.

Bug: Swipe not working as expected
https://user-images.githubusercontent.com/85645967/206266309-42d8a0be-d1d1-48e5-a612-0e0d8d49b00a.mp4

@Santhosh-Sellavel i think I’ll need a drag and drop library to add swiping to mobile web. I assumed that wasn’t a requirement because of that, but I guess I’ll put it together if I have to.

@JediWattson I believe this should have worked no extra extensions here.
Bug: Correct image not downloaded
https://user-images.githubusercontent.com/85645967/206266837-14632205-4a59-4890-890b-97ad6931288f.mov

@Santhosh-Sellavel i think I’ll need a drag and drop library to add swiping to mobile web. I assumed that wasn’t a requirement because of that, but I guess I’ll put it together if I have to.

Drag drop is not needed, but I believe we are good with tap as we need tap/swipe on mWeb as per the expected result here, so that should do for now. Let's wait for @chiragsalian thoughts here

@JediWattson
Please make sure the following items are handled

The author checklist is unfilled.
Update the latest recordings on all platforms
Update test/QA steps to verify this checklist.

@chiragsalian Please loop in the design to check if are we fine with the arrow placements and colors.

@Expensify/design, can you check this comment to verify arrow placement and colors.

Yes swipe/drag-drop is not needed and just tap is fine. From the video shown by santhosh here it looks like swiping is acting funky. Can we just prevent swipe animation there?

How tall are the buttons? It might be nice to use the same style as our FAB where we have a 52x52 button with a 20x20 icon inside? This way we get perfectly round buttons.

@Santhosh-Sellavel can you try the swiping again, that should work better now. I also set the buttons to not show when there's no next or previous attachment. I'll look into that file not loading, but I think the jpeg.jpg name is causing the issue.
@chiragsalian I updated my branch with main and was able to get it working
@shawnborton I updated the buttons as needed here

Also I'll try to get some new videos up in the next couple days

Please update video soon, we are already on a deadline we should it push it to get it merged ASAP.

Nice, thanks for updating from main. Its working better for me on web, desktop and ios.
Also @JediWattson i think you updated the style of the buttons to address shawn's comments but you'll need to update the screenshots and video in your PR description so that shawn can verify.

@JediWattson Please resolve conflicts and update the video soon, thanks!

@Santhosh-Sellavel I just pushed some changes that might fix the attachments downloading incorrectly, but I'm unable to test because of some api issue that seems internal (I checked the slack channels and verified this on my end). Soon as it's back I can continue to bring this to a finish, but it might be past the deadline set.

Please let me know when there are updated screenshots/videos to review!

All tests here are passing
The attachment download is working now, the swipe is still acting a bit weird some images getting resized.
https://user-images.githubusercontent.com/85645967/207125754-2934d7ae-53da-46e7-a658-392d15ad6cb8.mp4

And just to confirm, tapping once on the image will hide the arrow keys too?

@luacmartins @Santhosh-Sellavel I'm still blocked with this api issue, but I'll check in tomorrow to see where it's at

Left a few minor comments. I'm having issues testing these changes, since the images don't seem to load (see video). Am I missing something?

@luacmartins, i had the exact same problem as you earlier but it was fixed later. Are you sure you are up to date with the branch when you tested? I just retested with the latest code and i see the images in the carousel just fine.

I just merged with main here to keep it updated, but still having issues logging in. I filled out a form to hopefully mitigate it.
@Santhosh-Sellavel @chiragsalian

i had the exact same problem as you earlier but it was fixed later. Are you sure you are up to date with the branch when you tested? I just retested with the latest code and i see the images in the carousel just fine.

@chiragsalian I'm still seeing this issue. Here's what I tried:

Deleted my local branch
Fetched this branch again
Ran npm i
Ran npm run web
No images load 😞 No console errors nor failed network requests

I must be missing something obvious.

npm run clean didn't fix it 🤔

@luacmartins Have you checked on desktop or native platforms?

Yea, this is happening on all platforms:
Desktop
https://user-images.githubusercontent.com/22219519/207707179-8a424151-c464-4f78-b3eb-0dbc46d01945.mov
iOS
https://user-images.githubusercontent.com/22219519/207707187-eb281d0a-b638-4a34-a29c-bf65e2cc5523.mov
Android
https://user-images.githubusercontent.com/22219519/207707197-99695a22-4806-492b-82ef-dc9fd4af78a7.mov

@luacmartins
Here I just clone, it works for me on desktop
https://user-images.githubusercontent.com/85645967/207710221-7daaed4b-dfa7-442c-a0df-742993e5be39.mov

Yea, I must be holding something wrong 😞

@JediWattson Here we have a thread for workaround, kindly ping here once you fill the form. Once changes are done, re run web its should work.

All tests here are passing The attachment download is working now, the swipe is still acting a bit weird some images getting resized.
Simulator.Screen.Recording.-.iPhone.14.-.2022-12-12.at.23.59.01.mp4

Any update on this issue, this is not blocking as its on native?

No console errors nor failed network requests

You might want to make sure you are tracking All network requests and not just XHR since this is not an XHR request.
It seems to be working fine for me on desktop,
https://user-images.githubusercontent.com/14356145/207729646-ad39d7e7-6ab6-433a-8aac-6acd7fde03fd.mov

@Santhosh-Sellavel I did fill out that form and the app started working again!!
For the image resizing I noticed I could unset the height and width before retrieving the new image's height and width, which will trigger the spinner until the dimensions are fetched.
I'll try and get videos up soon as well!

@chiragsalian figured out the cause of the issue! I'm using ngrok which changes the sourceUrl for the image - you can see below that the urls are different.
That means that this conditional is always false, page is never set and we don't get the sourceUrl nor set the state resulting in an empty modal.

@JediWattson bump

@Santhosh-Sellavel I needed to update my ip to be whitelisted but I'll keep an eye on it

@Santhosh-Sellavel I needed to update my IP to be whitelisted but I'll keep an eye on it

@JediWattson Can you resolve conflicts, I believe the Ip whitelist is done periodically you should be able to get it. Please follow the thread to get it done.
Can you update the screen recording for other platforms and update PR descriptions based on the latest template.

@Santhosh-Sellavel I resolved conflicts, I got a form in and I'm checking out the template

Also, what's the status wrt updating the screenshot and videos @JediWattson, is it done such that we can review it, or are we still waiting for this? I see the description has been updated but usually we wait for a comment confirmation before we review so that we don't review things prematurely. Let us know.

@chiragsalian I pushed some changes that should get the config env working!
Also I need to get this android demo video up and that should all set there!!!

@chiragsalian @Santhosh-Sellavel I got the last video demo up!

@JediWattson Please resolve conflicts

@chiragsalian sweet, the branch is is now conflict free!

Will test further later today.

I noticed this weird issue focus issue
Steps I performed

Open attachment and navigate by clicking on arrows shown when hovered.
Use keyboard left or right arrow to cycle through attachments
Observe works as well without any issues.
Now closing the attachment viewer
Open any attachment again.
This time straight away use keyboard to navigate
Observe weird focus outline is set out side for the page

https://user-images.githubusercontent.com/85645967/211065432-8b91560a-60fb-4fd8-a02f-4fc126195fbc.mov
Simlarly

Open any attachment again.
This time click outside the image (i.e outside zoomable area refer attachment) and  use keyboard to navigate
Observe weird focus outline is set on the attachment view

https://user-images.githubusercontent.com/85645967/211066813-c35b9e53-057f-40e2-bd8c-bcce4f183e04.mov
cc: @chiragsalian

@JediWattson is this ready for another review? Did you have a chance to look into the issue @Santhosh-Sellavel described?

Yup, I can reproduce both the same focus view issues santhosh mentioned. Can you address that @JediWattson?
Basically whenever i use keyboard i see a weird blue line focus around the app. If i click someplace in the attachment carousel and then use keyboard i see a weird blue line focus inside the attachment model that's around the image.
Also the latest code you pushed has a wrong reference to canUseTouchscreen so the app is currently broken. GH lint shows the same error in your code. There are also comments by carlos that haven't been tackled. Let us know once these are addressed and when its ready for review.

I’m going to see if I’m able to finish this up later this evening

All right, updated my branch and I think I got the comments from @luacmartins, although I was wondering what the bump comment meant lol?
@Santhosh-Sellavel I looked into the outline and I think this is something involving the index.html there's some css there that sets this. Also I tested it on main with the modal and the same thing happens.

@Santhosh-Sellavel I looked into the outline and I think this is something involving the index.html there's some CSS there that sets this. Also, I tested it on the main with the modal and the same thing happened.

Can you elaborate more on it what sets it and why? Based on that we can decide whether we need to look into this or not, thanks!
@chiragsalian @luacmartins Anything you would like to add?

@chiragsalian or @luacmartins please initiate workflow tests, please review and test this one if you have time. I'll continue testing tomorrow thanks!

Also, I tested it on the main with the modal and the same thing happened.

What steps did you use to reproduce this issue on main?

although I was wondering what the bump comment meant lol

The bump comment referred to this comment

@JediWattson Bump

Can we get this moving forward, can you please post an update here, thanks!

I just pushed changes that should bring this home. Turns out the highlight can be prevented if blur is called on the ref from the event. I'll be attentive if you have any feedback on that to and make sure this finally gets merged!!!

Latest Screenshots/Videos

Web
https://user-images.githubusercontent.com/85645967/212564805-9b10915f-eca6-4e5b-a925-a0bc4a9e7724.mov

Desktop
https://user-images.githubusercontent.com/85645967/212564817-7f62d9cf-4df3-46b3-a85b-be9da395ec44.mov

Mobile Web - Chrome
https://user-images.githubusercontent.com/85645967/212564848-379faf2c-3b0c-4a14-9eab-392050f0acb9.mov

Mobile Web - Safari
https://user-images.githubusercontent.com/85645967/212564836-d67aabb7-5dd0-43b0-8b97-64c591eee9f3.mov

iOS
https://user-images.githubusercontent.com/85645967/212564841-80bd7c8e-ce08-4e1a-b339-676b50fcafa1.mov

Android
https://user-images.githubusercontent.com/85645967/212564843-fccb1868-694e-4d45-bb78-213fbe638d49.mov

Reviewer Checklist

[ ] I have verified the author checklist is complete (all boxes are checked off).
[ ] I verified the correct issue is linked in the ### Fixed Issues section above
[ ] I verified testing steps are clear and they cover the changes made in this PR

[ ] I verified the steps for local testing are in the Tests section
[ ] I verified the steps for Staging and/or Production testing are in the QA steps section
[ ] I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
[ ] 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)

[ ] I checked that screenshots or videos are included for tests on all platforms
[ ] I included screenshots or videos for tests on all platforms
[ ] I verified tests pass on all platforms & I tested again on:

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

[ ] If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
[ ] I verified proper code patterns were followed (see Reviewing the code)

[ ] 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).
[ ] I verified that comments were added to code that is not self explanatory
[ ] 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.
[ ] I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
[ ] I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
[ ] 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.
[ ] 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.
[ ] I verified the JSDocs style guidelines (in STYLE.md) were followed

[ ] If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
[ ] I verified that this PR follows the guidelines as stated in the Review Guidelines
[ ] I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
[ ] I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
[ ] 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
[ ] If a new component is created I verified that:

[ ] A similar component doesn't exist in the codebase
[ ] All props are defined accurately and each prop has a /** comment above it */
[ ] The file is named correctly
[ ] The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
[ ] The only data being stored in the state is data necessary for rendering and nothing else
[ ] 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)
[ ] 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)
[ ] All JSX used for rendering exists in the render method
[ ] 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

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

[ ] 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

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

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

[ ] 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)
[ ] 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.
[ ] 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.
[ ] I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

Can you complete the author checklist @JediWattson

all right, I think everything is cleared!

@JediWattson there's conflicts. Can you please merge main?
@Santhosh-Sellavel once the branch is updated, could you please finish the checklist?

Yeah sure, @luacmartins.

@luacmartins @Santhosh-Sellavel I resolved the conflicts, but I was unable to test iOS at this time. I can try tomorrow at sometime if no one else gets to it. I'm hoping it works still though.

If all is good will complete the checklist today!

If all is good will complete the checklist today!

Rapidly taping next arrow caused this crash
https://user-images.githubusercontent.com/85645967/213296581-a199c092-e78a-4a2b-9d72-e42a187e5f42.mov

Should we do something about this one?
Offline Image

Offline PDF

I think we should address those! @JediWattson could you please take a look?

On this one, what are we expecting? I think we need to provide a bit more colour there. On a first blush, I would say we should address it separately and probably after the image caching PRs for native and web are done, because isn't that why the image previews aren't loading offline? 😕

I like create a separate issue for that one as well and getting this PR to the finish line.

So two issues were reported,

https://github.com/Expensify/App/pull/9279#issuecomment-1396100307 - i think this can be solved via a debounce or something similar. Can you investigate @JediWattson.
https://github.com/Expensify/App/pull/9279#issuecomment-1396101732 - this issue is unrelated to this PR. The components show the exactly same view for images and pdf on main. So yes, let's create a separate GH issue for this issue.

@JediWattson bump

@Santhosh-Sellavel I got those changes in. Now I need to fix that issue with android.

Thanks for addressing those @JediWattson! Let us know when you are done addressing the android issue!

@JediWattson Bump!

@JediWattson Any update, also I noticed most of your commits gone unverified

@JediWattson you'll need to sign your previous commits. You can try the command below, using c2161645acff751082590b2f49e3b2760a1c70d1 as the commit hash, since that's the first unverified commit hash.
git -c core.editor=true rebase --exec 'git commit --amend --no-edit -n -S' -i c2161645acff751082590b2f49e3b2760a1c70d1

@JediWattson could you please share an update? Let's get this merged!

I'm really trying to figure out how to get these verified now

@JediWattson you'll need to sign your previous commits.

After trying this, if it does not work please ask in #expensify-open-source. There are many contributors that have come across the same issue so they can help out in a faster manner. Feel free to tag us also there so that this can be solved in realtime.

In addition to Chirag's comment above, we seem to have some lint issues now.

@chiragsalian I'm back on track after a squash here along with fixing the linting error. I'm looking into this android issue, and I found this comment that points out a variable that might be NaN or Infinity breaking react-native's function.

@Santhosh-Sellavel can you test that again. I think I got that issue fixed

Still happens
https://user-images.githubusercontent.com/85645967/214961317-58f8e7bb-e67a-46db-b493-6b640cb0b9c9.mov

@Santhosh-Sellavel give it another shot, I think I got it. It should be a lot faster too 😁

I think that issue is resolved now, or at least I couldn't reproduce it anymore. @Santhosh-Sellavel can you please check?

Yeah, I tested quickly the crash seems to be fixed. But I would like to test more to ensure the new code does not cause any issues! @luacmartins @chiragsalian

Swipe gestures are not available on mWeb Safari / Chrome. Is this intentional?

Yes, that's left out for now.

Cool, so we should just take a look at comment 1 above.

How we looking here? Does this require further changes or is it awaiting a re-review?

I think we are waiting for @Santhosh-Sellavel to re-review and @JediWattson to handle merge conflicts.
I'll re-review today also.

Cool, so we should just take a look at comment 1 above.

@JediWattson Is this issue solved?

@Santhosh-Sellavel  the comment should be fixed. I’ll get to the conflicts tonight

We have conflicts again

Just waiting for @JediWattson to address Chirag's review comments!

@Santhosh-Sellavel I updated the reports in attachment and reverted the code to match the current implementation

Code changes looks better to me. There is however one bug that i noticed. Steps: (tested on web)

Click on an attachment to open it in carousel. It loads properly.
Close the carousel. Click on the attachment again. Now i only see a blank screen and no arrow controls on hover.

@Santhosh-Sellavel fixed!

Thanks @luacmartins I will test shortly, meanwhile @JediWattson please update the videos in PR it's been three weeks!

Reviewer Checklist

[X] I have verified the author checklist is complete (all boxes are checked off).
[X] I verified the correct issue is linked in the ### Fixed Issues section above
[X] I verified testing steps are clear and they cover the changes made in this PR

[X] I verified the steps for local testing are in the Tests section
[X] I verified the steps for Staging and/or Production testing are in the QA steps section
[X] I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
[ ] 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)

[ ] I checked that screenshots or videos are included for tests on all platforms
[ ] I included screenshots or videos for tests on all platforms
[ ] I verified tests pass on all platforms & I tested again on:

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

[ ] If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
[X] I verified proper code patterns were followed (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] 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 verified that this PR follows the guidelines as stated in the Review Guidelines
[X] I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
[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] 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] I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

@Santhosh-Sellavel  it’s going to be a couple days before I can update these. Is this seriously a blocker if none of the ux/ui changed here???

Glitchy on Swipe
iOS
https://user-images.githubusercontent.com/85645967/217090482-6eaa6bd0-96ad-4293-b0eb-63892b17a949.mov

Deleted attachments are still shown
Steps

Open chats with attachment
Click on any attachment to view
Close the attachment view
Delete an attachment
Now open any other attachments
Cycle through you will still find the attachment in the carousel

https://user-images.githubusercontent.com/85645967/217097020-6a7c6641-cff9-4525-a5b2-a43f8562cae5.mp4

Tap/to hide is not working properly on native platforms
Steps:
issue 1

Open any image attachment for any chat
Double tap on it, it will zoom the image
Now tap single on the image arrow is not hidden

issue 2
Sometime tap itself not hiding/showing arrow weird
https://user-images.githubusercontent.com/85645967/217097906-20477bfc-24bf-4740-af7a-69d14228ccd2.mp4

I'm okay with creating a follow up for the glitch swipe issue if its too hard to tackle here.
However, i think the "deleted attachment" and "Tap/to hide is not working properly on native platforms" are more important issues to investigate. Is that something you can look into @JediWattson
Also thanks for the thorough testing @Santhosh-Sellavel.

Glitch on swipe - https://github.com/Expensify/App/pull/9279#issuecomment-1419797717
Delete attachments should not be shown - https://github.com/Expensify/App/pull/9279#issuecomment-1419832915
Tap to hide/show issues (Native only) - https://github.com/Expensify/App/pull/9279#issuecomment-1419841680

I think (2) should be addressed, let me know your thoughts on the other two. I also noticed that the attachment download success/error popup is not styled properly but I think there might be a separate issue for that, if not I'll report it on slack

Will continue testing, tomorrow!
cc: @chiragsalian @luacmartins

Yah I’ll try to take this on tomorrow evening

@Santhosh-Sellavel @chiragsalian I got the glitchy images fixed and the deleted comments.
The problem with the arrows when zoomed in is that there's another pan responder taking over when it's zoomed. I guess one solution would be to pass the event handler down to this pan responder and call it from there if it's not a double tap.

The problem with the arrows when zoomed in is that there's another pan responder taking over when it's zoomed. I guess one solution would be to pass the event handler down to this pan responder and call it from there if it's not a double tap.

I think this solution sounds good. @JediWattson do you wanna take a stab at it?

Sounds good to me too, please implement the changes and let us know!

@Santhosh-Sellavel @luacmartins all set!

@Santhosh-Sellavel wanna give this another go?

Yes, will do it  in few hours

@JediWattson we have conflicts again. Would you mind updating the branch?

@luacmartins @Santhosh-Sellavel resolved!!

@Santhosh-Sellavel did you get a chance to review the latest changes?

On it now!

Weird zooming issue while switching icon
Steps

Open a chat with multiples images
Open an image
Click on the image once to zoom
Click next/prev

Next/prev image should not be zoomed, actual zoom increases while cycling between images on new image
https://user-images.githubusercontent.com/85645967/218550285-7c881c5b-21f2-47b7-a100-f5dcfad7654e.mov

On native Swipe does not work on PDF attachments, but I think that's fine as we have arrows to switch between attachments. If feel it's crucial we need to create a follow-up.

@Santhosh-Sellavel fixed #2

I believe you are referring to 1!

@Santhosh-Sellavel that’s correct!

Is this ready for another round of review?

I think so. I took care of issue 1 and issue 2 seems like a whole other
thing
On Tue, Feb 14, 2023 at 8:16 AM Carlos Martins @.***>
wrote:

Is this ready for another round of review?
—
Reply to this email directly, view it on GitHub
https://github.com/Expensify/App/pull/9279#issuecomment-1429733021, or
unsubscribe
https://github.com/notifications/unsubscribe-auth/ABOV6DLL7ZKBNEPHIT5EB3DWXOARPANCNFSM5XSO6BTA
.
You are receiving this because you were mentioned.Message ID:
@.***>

Cool i'm fine with creating a follow-up issue for #2.

@JediWattson
For ## https://github.com/Expensify/App/pull/9279#issuecomment-1428499503
The image is now reset properly for this but the loading icon still moved up!

@Santhosh-Sellavel @luacmartins  this is only being worse at this point like 70% of what I’m working on here is tech debt and I bet when I fix this some other issue is going to pop up to delay this. Why couldn’t this get passed yesterday???

@JediWattson I understand that the back and forth can be really frustrating, especially when a lot of it is just resolving conflicts. We've been doing our best to review and test this PR in a timely manner, but given its nature, we've been testing many edge case scenarios to avoid regressions which has taken quite some time. I think we are really close to the finish line though, so let's keep up the pace and get this merged soon!

Yup the back and forth can be frustrating but I also wanted to say thank you @JediWattson for pushing the code forward and thank you @Santhosh-Sellavel for the strict review and catching edge cases to make sure we keep up our standards. Santhosh is doing the bulk of our testing and I understand that it can be very time-consuming for him as well.

@luacmartins @chiragsalian it can be a little daunting because sometimes these things can take all evening. I'll get an email about your comments on this PR at work and I go "oh no there goes my evening 🥲" luckily it is getting easier and I definitely feel like this is almost there!

I'm getting this warning while switching/navigating between images on web/Desktop
blob:http://localhost:8081/4505f58a-3464-48c4-a54b-d0b43b2fa5d5:189 Uncaught (in promise) Error: Worker was terminated
    at ensureNotTerminated (blob:http://localhost:8081/4505f58a-3464-48c4-a54b-d0b43b2fa5d5:189:17)
    at onFailure (blob:http://localhost:8081/4505f58a-3464-48c4-a54b-d0b43b2fa5d5:429:11)

https://user-images.githubusercontent.com/85645967/219782258-8dcc0dfa-ad05-427f-aacc-71314366cfe7.mov

I could reproduce the same error santhosh is failing. Sort of an edge case but it would be good to address.
I get it when clicking on an attachment on desktop and clicking prev button repeatedly at a fast speed.

@JediWattson any update on this https://github.com/Expensify/App/pull/9279#issuecomment-1435192338?

@Santhosh-Sellavel that's when you load a pdf. Apparently the way this was loaded was updated to use a specific import for Webpack 5. Should be all good now!!!

Still image not loading issue occurs to me
https://user-images.githubusercontent.com/85645967/220733967-5f90e571-3886-4df1-a103-6a324b6bc89e.mov

@Santhosh-Sellavel are there errors for you with that?

No errors

PDFs are not loading @JediWattson

@JediWattson Can you please do a sanity tests and confirm it doesn't break anything before marking it ready for review

@Santhosh-Sellavel the image loading issue should be fixed now. I reverted the PDF changes because I didn't know that it was going to break with your build. I can't test on your build and they're two completely different beasts, which is why the PDF changes worked for me but not for you.

@chiragsalian @Santhosh-Sellavel the pdf error is on main as well, and I'm not sure how to go about it without looking at web pack more

@JediWattson So far I'm not facing loading issue, I think its solved.

@Santhosh-Sellavel the pdf error is on main as well

Yeah may be, but I was not able to reproduce there.

Okay @JediWattson Can you please update the PR description with latest template & update screen recordings. If we don't run into anything major, let's possibly get this merged tomorrow, thanks!
cc: @chiragsalian

linter is failing so you've got some code to correct.

Reviewer Checklist

[x] I have verified the author checklist is complete (all boxes are checked off).
[x] I verified the correct issue is linked in the ### Fixed Issues section above
[x] I verified testing steps are clear and they cover the changes made in this PR

[x] I verified the steps for local testing are in the Tests section
[x] I verified the steps for Staging and/or Production testing are in the QA steps section
[x] I verified the steps cover any possible 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 checked that screenshots or videos are included for tests on all platforms
[x] I included screenshots or videos for tests on all platforms
[x] I verified tests pass on all platforms & I tested again on:

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

[x] If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
[x] I verified proper code patterns were followed (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] 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 verified that this PR follows the guidelines as stated in the Review Guidelines
[x] I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
[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] 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 reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web
https://user-images.githubusercontent.com/85645967/221285162-8dfec4a7-7ea2-408d-8db2-698aef7d9011.mov

Mobile Web - Chrome
https://user-images.githubusercontent.com/85645967/221285557-b6396e7a-2f69-47b3-b584-d1efb60e6f3e.mp4

Mobile Web - Safari
https://user-images.githubusercontent.com/85645967/221285483-ec228f00-15af-48a7-a9cb-74b7515e3921.mp4

Desktop
https://user-images.githubusercontent.com/85645967/221285204-32a621ba-0172-415c-bae5-1bd71594b1b0.mov

iOS
https://user-images.githubusercontent.com/85645967/221285462-50bdd078-687a-4d5f-a7ec-9f26f6471c12.mp4

Android
https://user-images.githubusercontent.com/85645967/221285583-e551fb09-3b2a-4ad5-b908-d50eb8e2d81b.mp4

@chiragsalian All good now for I just noticed something, I am okay with it but want to get clarified it with you too.
While old images from old chat history are loading, the prev button is clickable or there is no sign to the user that there is now loading
https://user-images.githubusercontent.com/85645967/221295215-c80e9249-e6b4-4707-a4fd-4fa05719da7a.mov

@Santhosh-Sellavel I ended up reverting those last two commits because it introduced most of the comments you made on the changes. I added some logic to check page is set as a number before showing the arrows (to prevent navigating with an undefined value). And I removed the logic to fetch oldest reports.
cc: @chiragsalian @luacmartins

And I removed the logic to fetch oldest reports.

I see this code still in your PR which ends up fetching older reportActions. Did you mean something else? If so can you still update that block based on the discussions of the thread.

@JediWattson bump

@JediWattson I'm not sure whether this is ready for you again, but I request you to test the basic cases and ensure everything is working as expected and also re-record the screens mostly that's when you can catch bugs.
Just did some testing though
No loading, the screen is blank for a while navigating through attachments
https://user-images.githubusercontent.com/85645967/221996952-22ef193f-8361-4a3c-818b-2b2a5e48b679.mp4
Let us know when this is ready for the Final review, thanks!

@Santhosh-Sellavel it should be good to go now!

@luacmartins that should be good now!

@JediWattson I'm not sure whether this is ready for you again, but I request you to test the basic cases and ensure everything is working as expected and also re-record the screens mostly that's when you can catch bugs.

@JediWattson I believe this is not done yet!

Seems the branch has conflicts, but I've been already testing for the last couple of hours and merging the main with this commit.  Mostly looks good and tests well.

I've been able to catch this bug again.
https://github.com/Expensify/App/pull/9279#issuecomment-1430550742

https://user-images.githubusercontent.com/85645967/223190670-6ec65709-cdb2-44f1-9f63-c66ce03f08b7.mov

Weird issues alone on android alone  (This could be a follow-up as this occurs only on android, we didn't modify much here platform-specific and I'll leave the final call to you @chiragsalian )

PDF Loading Failed when fastly navigated to the PDF Attachment
The image is loading every time when navigating back and forth.  The image is loaded for a while and

https://user-images.githubusercontent.com/85645967/223190194-3f2de491-6082-4f9d-a678-8c6d9ea55b21.mp4
then replaced with a loading page stuck with it.
cc: @chiragsalian

@Santhosh-Sellavel  I think the screen flickering is an android emulator issue: https://issuetracker.google.com/issues/253964623
can you explain how you're getting that first bug? I'm having trouble reproducing it locally.

@Santhosh-Sellavel I think the screen flickering is an android emulator issue: https://issuetracker.google.com/issues/253964623

I'm not using an emulator for android. It's a real device.

can you explain how you're getting that first bug? I'm having trouble reproducing it locally.

Sign out and log in again to perform the steps. The loading icon is placed weirdly based on the zoom. Check below the Icon moved towards the left.
https://user-images.githubusercontent.com/85645967/223202471-e628d3a1-1098-46ae-8427-ed577c7645ed.mov

@Santhosh-Sellavel I see what you mean and I think I made the loading state better on both web and mobile now. I'll work on the videos a little later and start uploading them!

Reviewer Checklist

[x] I have verified the author checklist is complete (all boxes are checked off).
[x] I verified the correct issue is linked in the ### Fixed Issues section above
[x] I verified testing steps are clear and they cover the changes made in this PR

[x] I verified the steps for local testing are in the Tests section
[x] I verified the steps for Staging and/or Production testing are in the QA steps section
[x] I verified the steps cover any possible 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 checked that screenshots or videos are included for tests on all platforms
[x] I included screenshots or videos for tests on all platforms
[x] I verified tests pass on all platforms & I tested again on:

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

[x] If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
[x] I verified proper code patterns were followed (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] 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 verified that this PR follows the guidelines as stated in the Review Guidelines
[x] I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
[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] 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 reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web & Desktop
https://user-images.githubusercontent.com/85645967/223506710-68fe0825-c9d6-43e3-9a31-a2d80b1e46cc.mov

Mobile Web - Chrome
https://user-images.githubusercontent.com/85645967/223506801-11550d2c-8871-419b-97b0-63c69802ef4b.mov

Mobile Web - Safari
https://user-images.githubusercontent.com/85645967/223508355-90677b4c-1efb-4e84-b38b-7fc75898f106.mp4

iOS
https://user-images.githubusercontent.com/85645967/223507263-d5cc839e-43af-400c-9e6c-4f0b7a4b6517.mp4

Android
https://user-images.githubusercontent.com/85645967/223507984-cb7b9239-724d-4be8-af58-a79e9b95b986.mp4

While testing today I found a Bug: large PDF files with 1000+ pages cause the app to freeze, this happens on the main too!

@Santhosh-Sellavel I think that's an edge case that we could tackle as a follow up and as mentioned might happen on main too.

🚀 Deployed to staging by https://github.com/luacmartins in version: 1.2.81-0 🚀

platform
result

🤖 android 🤖
success ✅

🖥 desktop 🖥
success ✅

🍎 iOS 🍎
success ✅

🕸 web 🕸
success ✅

Awesome work getting this to production! I've been enjoying this one, really great improvement.

👋 Hey all, dropping in to let you all know that tooltips weren't added for the < and > arrows when introducing this feature, resulting in this bug. Please keep in mind accessibility going forward and remember to add tooltips where appropriate! Thanks 🙇

This issue was not handled in this PR. Image automatically changes when carousel is open and a new image is received in the chat.


Privacy policy located in different place

In windows 10, privacy policy is located under
User Configuration\Administrative Templates\Microsoft Office 2016\ Tools | Options | General | Service Options… \ Online Content.
i enabled online control options and service level options
Please include  send personal information must be disabled in gpo, which is located under
User Configuration\Administrative Templates\Microsoft Office 2016\ Privacy\Trust Center.
Also CEIP located under User Configuration\Administrative Templates\Microsoft Office 2016\ Privacy\Trust Center.

Document Details
⚠ Do not edit this section. It is required for docs.microsoft.com ➟ GitHub issue linking.

ID: 047d2b74-e963-a006-cb74-29257c409647
Version Independent ID: 4f274c79-18ae-2c36-19a3-cfb73d7e6cf3
Content: Privacy options in Office 2016
Content Source: DeployOffice/security/privacy-options-in-office.md
Service: o365-proplus-itpro
GitHub Login: @DHB-MSFT
Microsoft Alias: danbrown

The paths listed in the article are for admins that are using Group Policy with Active Directory on Windows Server. That's why they include \Policies\ in the path. If you're editing the local policy on Windows 10 using the Local Group Policy Editor, \policies\ isn't included in the path.
Thanks for your suggestion about adding information about "Send personal information"


Move common properties to automatically-imported files

The error is due to deprecation - which would have given us a warning before (I'm surprised we didn't see it) and it's now an error. This is a good thing, catching warnings...

Hmm... that's not actually it. It's due to Microsoft.DotNet.Analyzers.Compatibility.
It's easy to fix though - done in another commit.

Thanks - will merge on green, but definitely not before, and will add a new analyzer issue.

Hmm... the error with Firestore will need a bit more investigation.


Fix APCPasscodeViewController

it’s still used if your passcode is old enough to pre-date when mPower started using ORKPasscodeViewController for new passcodes.

Don’t try to set the first responder until the animation has finished
Fix layout of “Forgot passcode?” button so it’s pinned to just above the “Use TouchID” button and not the bottom of the view, behind the keypad, where it’s always inaccessible

Also fix the error log formatting for when looking something up in the keychain fails, so we actually get a somewhat useful message.

Looks good to me


Add support for boolean columns to Arrow FSI

@paul-aiyedun I fixed the comments, PTAL.

I'm seeing an issue in this PR when running the tests:
2021-07-19T21:19:26.030229 I 1322788 0 QueryRunner.cpp:785 stdlog runSelectQuery 96 47 omnisci admin 909- {"query_str"} {"SELECT val2 FROM fsi_nulls_boolean order by id;"}
2021-07-19T21:19:26.030323 1 1322788 QueryRunner.cpp:785 stacked_times runSelectQuery 96 total time 47 ms
  processImpl<PHONE_NUMBER>19008 - total time 3 ms
2021-07-19T21:19:26.030353 1 1322788 0 QueryRunner.cpp:484 jsonlog { "name": "CPU Memory Info", "TotalMB": 64291, "FreeMB": 52530.7, "ProcessMB": 436.234, "VirtualMB": 111888, "ProcessPlusSwapMB": 211.551, "ProcessSharedMB": 224.684, "FragmentationPercent": 67 }
2021-07-19T21:19:26.030901 F 1322788 0 ArrowCsvForeignStorageTest.cpp:67 Check failed: exp == v<T>(crt_row[0]) (1 == 0)
2021-07-19T21:19:26.031351 I 1322788 0 Calcite.cpp:649 Shutting down Calcite server
2021-07-19T21:19:26.033834 I 1322788 0 Calcite.cpp:661 shut down Calcite
shutdownSubsystemsAndExit: Interrupt signal (6) received.

the issue only seems to happen intermittently on our build machines, but I can reproduce it using Arrow 4.0 every time . Thoughts?

Hmm, I'll take a look, thanks.

@alexbaden Unfortunately, I was unable to reproduce the issue. Could you please share, how you build and execute tests?

It looks like this pr is still not in master. @alexbaden @paul-aiyedun could you please take a look?

Hey @mattpulver, I found a problem with no-nulls path, but it shouldn't really affect nulls test, I also rebased the branch on current master, so can you please rerun the CI to check how it works.
I couldn't really reproduce the issue even when compiling with clang.

@fexolm Looks like the recent updates fixed the previous clang error. We're reviewing it now on our internal repo w/ @paul-aiyedun's help.

Merged: https://github.com/omnisci/omniscidb/commit/0517d99b467806f6af7b4c969e351368a667497d


[CNSMR-1713] Custom Input should work with AnyRenderable.

https://babylonpartners.atlassian.net/browse/CNSMR-1713
Bento Custom Input operator should work when decorating the type-erased AnyRenderable. This is currently crashing since statically speaking AnyRenderable.View is not the same as its wrapping value's View type.

BTW are similar fixes also needed for other compositions e.g. DeletableComponent ?

I'm starting to feel that adding ViewStorageOwner for properly handling didMount / willUnmount is a separate issue from this PR's objective.
It is bringing another complexity since #162 , and as I also questioned in https://github.com/Babylonpartners/Bento/pull/179#issuecomment-497364809 , there may need more relevant fixes.
There's also a question about if we can simplify code as follows, eventually removing ViewStorage:
https://gist.github.com/inamiy/4d3d8a9434cb7bcab4baf370fac647d0
(but I don't want to discuss this further in this PR)
Thus, can you split the PR to only focus on CustomInput fix for now?
Or, I can't approve until all irrelevant questions are discussed here and answered.

@inamiy Adding ViewStorageOwner is essential. Without it, either composite components conform their views to BentoReusableView, or none of them can construct ViewStorage.

Removed ViewStorageOwner. Extend instead BentoReusableView to support the use cases. I suppose making BentoReusableView public might be a better choice after all.


Made some changes which I think are important:

i) Made the code colorful: Used the colored module which changes the text color thus beautifying the code output
ii) Made the full function inside try and except block to avoid errors
I tested it and it worked fine. If it raises any error for you, please notify me. I will try to solve it. You might face one common error:
ENABLE_WINDOWS_TERMINAL_MODE error.
This can we solves by enabling windows terminal mode for python (this error is raised by colored module)

Added Your name to the contributors 🎉

Added Your name to the contributors 🎉


Coloring labels by a continuous variable fixed

Codecov Report
Attention: Patch coverage is 91.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 84.32%. Comparing base (ffd4a1f) to head (487f6a5).

Files with missing lines
Patch %
Lines

src/spatialdata_plot/pl/utils.py
91.66%
1 Missing :warning:

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #344      +/-   ##
==========================================
+ Coverage   84.27%   84.32%   +0.05%
==========================================
  Files           8        8
  Lines        1558     1557       -1
==========================================
  Hits         1313     1313
+ Misses        245      244       -1

Files with missing lines
Coverage Δ

src/spatialdata_plot/pl/utils.py
77.37% <91.66%> (+0.08%)
:arrow_up:

The two failing tests can probably be traced back to https://github.com/scverse/spatialdata-plot/issues/324


failed to run shell on windows.

I use windows powershell terminal to connect handy-sshd.
PTY allocation request failed on channel 0

I guess pty allocation is not supported in Windows:

https://github.com/creack/pty/issues/95

You can use it in WSL.


ember-cli-mirage does not need passthrough post 0.2.0-beta.2

With ember-cli-mirage#281 mirage shuts down its server at the end of each test.
This config should no longer be needed for apps using latest ember-cli-mirage and the destroyApp helper.
Tested on a newly created app.

thanks @kamalaknn - much appreciated

This is a great addon. Loving the updates on the latest release. Thank you all for the good work :+1:


Encoding and Decoding messages that include service definitions

Similar to #15 if I try to define a message that includes a service definition I get an CaseClauseError.
Reproducible test:
defmodule ServiceDefinitionsTest do
  use ExUnit.Case

  defmodule Proto do
    use Protobuf, """
      message Msg {
        optional string name = 1;
      }
      service MsgService {
        rpc Create (Msg) returns (Msg);
      }
    """
  end

  test "it can create an extended message" do
    msg = Proto.Msg.new(name: "Ron")
    assert msg == %Proto.Msg{name: "Ron"}
  end

  test "it can encode an extended message" do
    msg = Proto.Msg.new(name: "Ron")
    assert Proto.Msg.encode(msg) == <<10, 3, 82, 111, 110>>
  end

  test "it can decode an extended message" do
    encoded = <<10, 3, 82, 111, 110>>
    assert Proto.Msg.decode(encoded) == %Proto.Msg{name: "Ron"}
  end
end

I get these results
.

  1) test it can decode an extended message (ExtensionFieldTest)
     test/rpc_definitions_test.exs:25
     ** (CaseClauseError) no case clause matching: :service
     stacktrace:
       lib/exprotobuf/decoder.ex:9: anonymous fn/2 in Protobuf.Decoder.decode/2
       (elixir) lib/enum.ex:1261: Enum."-reduce/3-lists^foldl/2-0-"/3
       lib/exprotobuf/decoder.ex:8: Protobuf.Decoder.decode/2
       test/rpc_definitions_test.exs:27

  2) test it can encode an extended message (ExtensionFieldTest)
     test/rpc_definitions_test.exs:20
     ** (CaseClauseError) no case clause matching: :service
     stacktrace:
       lib/exprotobuf/encoder.ex:7: anonymous fn/2 in Protobuf.Encoder.encode/2
       (elixir) lib/enum.ex:1261: Enum."-reduce/3-lists^foldl/2-0-"/3
       lib/exprotobuf/encoder.ex:6: Protobuf.Encoder.encode/2
       test/rpc_definitions_test.exs:22

Finished in 0.1 seconds (0.1s on load, 0.00s on tests)
3 tests, 2 failures

Randomized with seed 298949

It looks like we can get the service defs from :gpb, so this one we can add to the TODO after extensions.

I was just about to start working on this tonight, but I realized that it is already supported. Something like this works now:
defmodule Message do
  use Protobuf, """
    message Greeting {
      optional string name = 1;
    }
    service HelloService {
      rpc hello(Greeting) returns (Greeting);
    }
  """
end

Then at runtime I can do:
iex(6)> Message.defs
[{{:msg, Message.Greeting},
  [%Protobuf.Field{fnum: 1, name: :name, occurrence: :optional, opts: [],
    rnum: 2, type: :string}]},
 {{:service, Message.HelloService}, [{:rpc, :hello, :Greeting, :Greeting}]}]

This gives me the ability to reflect on all my protobuf service definitions and see their respective types. Woohoo! Thanks @bitwalker

I hadn't revisited this since some recent changes were merged, I'm glad this is resolved already!


docs: format webhook json

What does this PR do?
webhook json格式化
Motivation
Additional Notes

感觉用 JSON 代码块更好些：
```json

```

https://gist.github.com/runeimp/f14a504f6cb402856c23d27decc133ac

对。已强制提交


Implement RLockAll()

This PR implements RLockAll() that acquires a priority read lock on all shards.

Pull Request Test Coverage Report for Build<PHONE_NUMBER>

58 of 62 (93.55%)  changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-6.2%) to 93.846%

Changes Missing Coverage
Covered Lines
Changed/Added Lines
%

smutex.go
58
62
93.55%

Totals

Change from base Build 965847044:
-6.2%

Covered Lines:
61

Relevant Lines:
65

💛  - Coveralls


Add configs for event.

use_tag_as_title
text_key

Could you write title of this PR in English and make commit message understandable by other people?

how is it?


Fabric table should be backed by a single backing store (attribute store), remove delegate callbacks #6419

false

https://github.com/project-chip/connectedhomeip/blob/f0f5de0d79ae1c51d7fddbf70900ebac0acd899a/src/credentials/FabricTable.h#L449-L454

This issue was generated by todo-issue based on a TODO comment in f0f5de0d79ae1c51d7fddbf70900ebac0acd899a.

Fixed here:
https://github.com/project-chip/connectedhomeip/issues/14119


State persistance when lazy-loading modular apps

I'm submitting a...

[x] Support request

I'm coming from the "redux world", looking for a simpler way to manage state in an ng6 app.
What I'm wondering - is akita able to handle modularized app state, especially regarding persistance?
As far as I understand the docs and code, there's only one call / config to persistState() in the app module where I could include / exclude parts.

Yes, so the state is rehydrated after a page refresh.

When you call persistState() without include/exclude, Akita will sync the whole stores into the local storage.

That's exactly the problem - when a lazy module isn't accessed yet, the stores shape isn't known as it should be configured within the module.

Sorry, I can't understand what you mean. When your lazy modules instantiate the store providers, Akita will sync them to the local storage.

Say I have three modules: App, Auth and Shop.
App and Auth get loaded on startup, Shop is lazy and defines it's own store that is provided when accessed for the first time.
Now the user hits refresh in his browser - how does akita handle the "Shop" store when the lazy module is not accessed?
Akita can't know it's shape or even if it exists, so will it be removed from localstorage upon next save (e.g. because "Auth" store has changed) or will akita simply keep all values as they are?
And how will it populate the stored values to the "Shop" store as soon as it is lazily loaded?
As I told in the first post I'm coming from redux where I've implemented a "provider solution" that checks localStorage whenever a new "store provider" is registered, so I wanted to know if akita is able to handle the same requirement.

OK, I see, I'll give it a try with a fresh project and test if it can replace the current redux implementation as it seems waaaaaay easier to handle.
Thanks for your time & great work :)

No problem. If you have any further questions, let me know. Good luck.


Marshaling [n]byte doesnt work

	type ByteSliceFormats struct {
		HexBytes       [4]byte `kdl:"bytes-hex"`
	}

	f := ByteSliceFormats{
		HexBytes:       [4]byte{24, 54, 12, 85},
	}

	if data, err := kdl.Marshal(f); err == nil {
		fmt.Println("kdl: ", string(data))
	}
	fmt.Printf("No kdl: %x\n", f)

kdl:
No kdl: {18360c55}

This is because, despite the name of your struct, HexBytes is a byte array, not a byte slice. Arrays weren't previously supported. Fixed in 21754ba.


Create proper README and other project documentation

Self explanatory

[x] README
[ ] Project documentation based on those from other UCCSER projects
[x] Consider LICENCE-THIRD-PARTY file & directory in the same style as other UCCSER projects
[x] Add contributors file/page similar to other UCCSER projects

Oh, like from uccer/cs-unplugged /docs/source?

@CyberFlameGO Correct. Similar structure and a lot of the content will be the same due to the similarity in the systems. This sadly just isn't high on our prority, especially as I'm the only dedicated technical developer currently.


FR: Unlimited length audio+text conditioning with generate_with_chroma()

Hi,
I've got a script going which takes an input audio, crops it into 30 second chunks, passes each one consecutively to generate_with_chroma() function and then concatenates the results.
Even though I have tried setting torch.manual_seed() and pytorch_seed.seed() to a fixed seed, it seems that each 30 seconds, the generation changes the style completely. Any hints onto how to make an audio input pass through the network and maintain the consistency throughout?
thanks.

Someone was already working on this & developed a fork. Perhaps you'll get more info from this conversation or working with them?
https://github.com/facebookresearch/audiocraft/issues/36#issuecomment-1586236702

Have you tested a plain text prompt to make sure the problem is the seed? It probably isn't working  working but just in case.
I haven't tried audiocraft, but in Bark but I had to do set a laundry list of stuff to get a seed. (Also it cuts speed by like 50%)

def set_seed(seed: int = 0):
    """Set the seed
    seed = 0         Generate a random seed
    seed = -1        Disable deterministic algorithms
    0 < seed < 2**32 Set the seed
    Args:
        seed: integer to use as seed
    Returns:
        integer used as seed
    """

    original_seed = seed

    # See for more information: https://pytorch.org/docs/stable/notes/randomness.html
    if seed == -1:
        # Disable deterministic

        print("Disabling deterministic algorithms")

        torch.backends.cudnn.deterministic = False
        torch.backends.cudnn.benchmark = True

        if "CUBLAS_WORKSPACE_CONFIG" in os.environ:
            del os.environ["CUBLAS_WORKSPACE_CONFIG"]

        torch.use_deterministic_algorithms(False)

    else:
        print("Enabling deterministic algorithms")

        torch.backends.cudnn.deterministic = True
        torch.backends.cudnn.benchmark = False

        os.environ["CUBLAS_WORKSPACE_CONFIG"] = ":4096:8"
        torch.use_deterministic_algorithms(True)

    if seed <= 0:
        # Generate random seed
        # Use default_rng() because it is independent of np.random.seed()
        seed = np.random.default_rng().integers(1, 2**32 - 1)

    assert 0 < seed and seed < 2**32

    np.random.seed(seed)
    random.seed(seed)
    torch.manual_seed(seed)
    torch.cuda.manual_seed_all(seed)
    os.environ["PYTHONHASHSEED"] = str(seed)

    print(f"Set seed to {seed}")

    return original_seed if original_seed != 0 else seed

The vanishing audio after extension was experienced & addressed in the comment I tagged before. I invite you to check that out.

https://github.com/rsxdalv/tts-generation-webui/blob/main/src/musicgen/musicgen_tab.py#LL172C22-L172C39 this works for me
https://github.com/rsxdalv/tts-generation-webui/blob/main/src/utils/set_seed.py#L8

Maybe this might help. Its not working completly because the audio is vanishing over time, but maybe a starting point. I could not get a complete 2 Minutes audio clip. After 1 minute the output is degrading.
`def generate_long_audio(model, initial_output, prompt_sample_rate=32000, total_length=120): """Generates a longer audio clip by repeatedly feeding the model with the last half of its output.
Args:
    model: the model used to generate the audio.
    initial_output (torch.Tensor): the initial audio clip.
    prompt_sample_rate (int): the sample rate used by the model.
    total_length (int): the total length of the audio clip to generate, in seconds.

Returns:
    torch.Tensor: the generated audio clip.
"""
# Convert total length from seconds to samples
total_length_samples = total_length * prompt_sample_rate
half_length_samples = 15 * prompt_sample_rate  # 15 seconds in samples

# Initialize the output with the initial audio clip
output = initial_output

while output.shape[-1] < total_length_samples:
    # Calculate the index at which to split the output
    split_index = max(output.shape[-1] - half_length_samples, 0)

    # Get the first and second halves of the output
    first_half = output[..., :split_index]
    second_half = output[..., split_index:]

    # Generate a continuation from the second half
    continuation = model.generate_continuation(second_half, prompt_sample_rate=prompt_sample_rate, progress=True)

    # Concatenate the first half of the output with the continuation to get a longer clip
    output = torch.cat([first_half, continuation], dim=-1)

    # Print the current length of the clip
    current_length_seconds = output.shape[-1] / prompt_sample_rate
    print(f'Current length of the clip: {current_length_seconds} seconds')

# If the output is longer than the desired length, trim it
if output.shape[-1] > total_length_samples:
    output = output[..., :total_length_samples]

return output`

It seems that you are missing "descriptions" on continuations, a.k.a the text prompt

Something changed with determenistic outputs, 0.0.1 worked but 0.0.2a2 only works for 2-3 seconds. https://github.com/facebookresearch/audiocraft/issues/111

Curious if anyone has gotten high quality results in this vein. I tried for a few hours and noticed that I could fix the petering out or fix the disjointedness, but not fix both. Has anyone produced arbitrary length clips of high quality that neither peter out nor lose track of the starting theme?


position: left, center, right

I seem not be able to make the position work. Nor with altposition.
Maybe i miss something the annoying thing is that the datepicker is close to the right browser border. And with center the picker is just over the viewable part.
I think left and right are switched. Entering position right give diserble effect. and moves popup to left.
(btw: this is a great datepicker)

Tnx for the compliment 😄 . I will check what is going on there

Turns out it is not an issue, since the menu will be aligned to the left/right border of the input element. In your case, right is the correct prop to pass