[$2000] Chat - Main composer is displayed when editing two messages

If you haven’t already, check out our contributing guidelines for onboarding and email<EMAIL_ADDRESS>to request to join our Slack channel!

Action Performed:
Case 1:

Send two messages (message A and message B)
Switch to edit mode in message A
Switch to edit mode in message B
Hide keyboard (press back button)
Main Composer is shown but it shouldn't because focus is still on message B

Case 2:

Send two messages (message A and message B)
Switch to edit mode in message A
Switch to edit mode in message B
Click the cancel button of message A
Main Composer is shown but it shouldn't because focus is still on message B

Case 3:

Send two messages (message A and message B)
Switch to edit mode in message A
Open EmojiPicker
Close EmojiPicker
Switch to edit mode in message B
Click the cancel button of message A
Main Composer is shown but it shouldn't because focus is still on message B

Notes:

For case 1 we may have a fix for that already (the fix didn't introduce a regression) (ref1, ref2)
For cases 2 and 3 we need to make sure we are not re-introducing the regression caught in #24016

Additional testing (regression) steps:

Send a message (message A)
Switch to edit mode in message A
Clear the edit field
Press save
Confirm deletion prompt
Verify Main Composer is shown because there is no other focused message

Expected Result:
Main compose box should not be displayed
Actual Result:
Main compose box is displayed
Workaround:
Don't edit two messages at once.
Platforms:

Which of our officially supported platforms is this issue occurring on?

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

Version Number: 1.3.47
Reproducible in staging?: Yes
Reproducible in production?: Yes
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
https://user-images.githubusercontent.com/93399543/232604613-64146c4c-9b6c-47c3-bbe3-839122ff6006.mp4
https://github.com/Expensify/App/assets/35863227/00f9d5e3-6bbf-44f3-a668-6d6dff8a1c83
Expensify/Expensify Issue URL:
Issue reported by: @s77rt
Slack conversation: https://expensify.slack.com/archives/C02NK2DQWUX/p1690561889854179
View all open jobs on GitHub
Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01230125fc5ec18c7d
Upwork Job ID:<PHONE_NUMBER>158468096
Last Price Increase: 2023-08-09

For context, we discovered and discussed about this issue here: https://expensify.slack.com/archives/C02NK2DQWUX/p1690561889854179

@s77rt Can you confirm if this issue is reproducible on other platforms? or just on Android?

Proposal
Please re-state the problem that we are trying to solve in this issue.
Main composer is displayed when editing two messages when the first one is cancelled if we open Emoji picker and close
What is the root cause of that problem?
When we cancel or save the draft, the following code runs
https://github.com/Expensify/App/blob/024d210bc0b5a0454b62a72de60057af67cdd045/src/pages/home/report/ReportActionItemMessageEdit.js#L123-L129
We consider the case when isFocusedRef.current is true. EmojiPickerAction.isActiveReportAction checks the last active report action
https://github.com/Expensify/App/blob/024d210bc0b5a0454b62a72de60057af67cdd045/src/components/EmojiPicker/EmojiPicker.js#L114
Even though we hide the emoji picker, last active report action still remains and this function returns true. So the if condition returns false and ComposerActions.setShouldShowComposeInput(true); is called
This is the root cause
What changes do you think we should make in order to solve the problem?
We can solve this issue by setting last report action to empty when we hide the emoji picker
Update the hideEmojiPicker method of [EmojiPicker] as follows
    const hideEmojiPicker = (isNavigating) => {
        if (isNavigating) {
            onModalHide.current = () => {};
        }
        emojiPopoverAnchor.current = null;
        setIsEmojiPickerVisible(false);
        setReportAction({});
    };

This works as expected

Result

What alternative solutions did you explore? (Optional)

@techievivek Yes, on all platforms (as long as the window width is small). Just a note the second video in OP does not reflect the current state. https://github.com/Expensify/App/pull/23618 is not deployed to staging yet.

@s-alves10 Thanks for the proposal. Your RCA is correct. The suggested solution although it looks like it would work it may cause a race condition ^1 ^2. The alternative solution looks more like a workaround (we are not clearing the report action in the right place).
It would better to fix the first solution race condition e.g. have ReportActionItemMessageEdit useEffect run before ReportActionItem or find another solution to https://github.com/Expensify/App/issues/22214.

Proposal
Updated

@dukenv0307 Thanks for the proposal. I don't think your RCA is correct. This issue only occurs after opening the emoji picker. Can you please update your proposal taking this into account?

Proposal
Please re-state the problem that we are trying to solve in this issue.
The Composer shows up even though the focus is still in message B. This happens when another message A had been edited before and had also used the Emoji Picker.
What is the root cause of that problem?
As already mentioned, the report action remains active even though the emoji picker is hidden. This causes the composer to show up.
What changes do you think we should make in order to solve the problem?
To solve the problem, whenever the ReportActionItemMessageEdit is unmounted, we can safely hide the EmojiPicker if this is an active report action. We can replace this code as follows.
This will fix the current issue and #22214.
            if (EmojiPickerAction.isActiveReportAction(props.action.reportActionID)) {
                EmojiPickerAction.hideEmojiPicker(true);
            }

            // Skip if this is not the focused message so the other edit composer stays focused.
            if (!isFocusedRef.current) {
                return;
            }

What alternative solutions did you explore? (Optional)

Proposal
Please re-state the problem that we are trying to solve in this issue.
The Composer shows up even though the focus is still in message B. This happens when another message A had been edited before and had also used the Emoji Picker.
What is the root cause of that problem?
As already mentioned, the report action remains active even though the emoji picker is hidden. This causes the composer to show up.
What changes do you think we should make in order to solve the problem?
To solve the problem, whenever the ReportActionItemMessageEdit is unmounted, we can safely hide the EmojiPicker if this is an active report action. We can replace this code as follows.
This will fix the current issue and #22214.
            if (EmojiPickerAction.isActiveReportAction(props.action.reportActionID)) {
                EmojiPickerAction.hideEmojiPicker(true);
            }

            // Skip if this is not the focused message so the other edit composer stays focused.
            if (!isFocusedRef.current) {
                return;
            }

What alternative solutions did you explore? (Optional)

@rojiphil Thanks for the proposal but isn't this proposed already by @s-alves10?

@rojiphil Thanks for the proposal but isn't this proposed already by @s-alves10?

But, he is also suggesting to add isEmojiActive. If that is required, then, his proposal is a good choice. I do not think that is required. That way, my proposal is different.

Hey, I have updated the issue description to include the original bug where the main composer gets focused when editing two messages.
👋 @garrettmknight coming from this bug https://github.com/Expensify/App/issues/17531#issuecomment-1662191253. This issue is linked to https://github.com/Expensify/App/issues/17531, which we decided to close due to the PR being reverted. Since the original issue is complex and was priced at $2000, I'll update this one to start with the same price.

@s77rt

@techievivek Yes, on all platforms (as long as the window width is small). Just a note the second video in OP does not reflect the current state. https://github.com/Expensify/App/pull/23618 is not deployed to staging yet.

I see, in that case, can you please ensure the proposals include fix for both emoji picker and the race condition bug. Also, let me know if you want me to update anything in the OP, thanks.

@s77rt
Do we need to update the issue description? I think it should include https://github.com/Expensify/App/issues/17531 as well

Yeah, we have 4 cases (at least) to handle:

2 mentioned here https://github.com/Expensify/App/issues/17531#issuecomment-1646614420
This one about the emoji picker
The forgotten one (regression) about saving an empty message https://github.com/Expensify/App/issues/24016

Will have to summarise those before updating the OP, but feel free to post proposals

Yeah, we have 4 cases (at least) to handle:

@s77rt   Thanks for the feedback and details. There is quite some history to this issue which I was unaware of. Will look at this more closely and see if I am able to propose something here.

@techievivek Please update the description to:
Action Performed:
Case 1:

Send two messages (message A and message B)
Switch to edit mode in message A
Switch to edit mode in message B
Hide keyboard (press back button)
Main Composer is shown but it shouldn't because focus is still on message B

Case 2:

Send two messages (message A and message B)
Switch to edit mode in message A
Switch to edit mode in message B
Click the cancel button of message A
Main Composer is shown but it shouldn't because focus is still on message B

Case 3:

Send two messages (message A and message B)
Switch to edit mode in message A
Open EmojiPicker
Close EmojiPicker
Switch to edit mode in message B
Click the cancel button of message A
Main Composer is shown but it shouldn't because focus is still on message B

Expected Result:
Main compose box should not be displayed
Actual Result:
Main compose box is displayed

Notes:

For case 1 we may have a fix for that already (the fix didn't introduce a regression) (ref1, ref2)
For cases 2 and 3 we need to make sure we are not re-introducing the regression caught in #24016

Additional testing (regression) steps:

Send a message (message A)
Switch to edit mode in message A
Clear the edit field
Press save
Confirm deletion prompt
Main Composer is not shown but it should because there is no other focused message

@s77rt Updated, thanks.

Proposal
Updated
@s77rt
I made a slight changes considering the various cases. It works fine on web platform. I'll test on all platforms soon

@s-alves10 Thanks for the update. I haven't read the proposal yet but at first glace looks like there is a lot of code :sweat_smile: It would be great if you can summarise the solution of each case in a sentence or two e.g. https://github.com/Expensify/App/issues/17531#issuecomment-1646614420. Regardless I will review asap. I'm also looking forward for platforms test.

@s-alves10 Any update on the above?

Will update soon

Proposal
Updated

@s-alves10 Thanks for the update.
Case 1
I think it's clear as we'd be reapplying the previous solution.
Case 2
Root cause
When we delete or unmount a message, we show the composer no questions asked
Solution
We should show the main composer only if we delete or unmount the active message. A message is considered active if:

It's focused: This is a straightforward check as we have a state that tracks the focus :heavy_check_mark:
It has context menu open: Let's discuss this point. We can check if the report action has context menu open using ReportActionContextMenu.isActiveReportAction but as I understand this does not work as expected because once we delete the message we clear the report action (too early) and when we check isActiveReportAction it returns false, we deleted an active message but lost the info that tells us that - this will make composer not show when needed (regression). Can you explain how we are going to handle this scenario?
It has emoji picker open: Hold

Case 3
Hold

@s77rt
Case 2
I don't suggest to clear the report action on unmount or delete. I am suggesting to clear the report action when other report action gets focused.

@s-alves10 I'm not saying that we should clear it. I'm saying it's already being cleared here right? So calling isActiveReportAction in ReportActionItemMessageEdit's useEffect may be too late.

@s77rt
In my proposal, I suggested to manage confirm delete modal and context menu separately. We need to check report action for confirm delete modal when checking if we should show composer

@s77rt
I was wrong. I think the above code should be deleted. We need to clear the reportActionID on focus event

@s77rt
I've tested all 4 cases mentioned above and works fine. Race condition won't be happen.
I'll find more edge cases.
By the way, can you tell me your thoughts on my solution?

@s-alves10 I would really prefer not to add another state management for context menu, there should be only one, the confirm modal is supposed to be a part of the context menu.

I think the above code should be deleted. We need to clear the reportActionID on focus event

I think it's more logical to clear the report action when the context menu is hidden. There can be cases where we just hide the context menu and not focus on another report action.

@s77rt
Here are my thoughts.

I would really prefer not to add another state management for context menu, there should be only one, the confirm modal is supposed to be a part of the context menu.

We have some interactions related to the report action in edit mode. They are emoji picker and delete modal.
We don't have context menu for actions in edit mode. So I wanted to separate context menu and delete modal, and to handle emoji picker and delete modal similarly. In addition, I think there is a big gap between context menu and delete modal. Why do they need to be handled as one state?

I think it's more logical to clear the report action when the context menu is hidden. There can be cases where we just hide the context menu and not focus on another report action.

I thought handling emoji picker and delete modal similarly would be good. For emoji picker, we have race condition if we clear the report action when it hides. So I suggest not to clear it on hide. I think similar race condition can be happened for delete modal as well.
These are just my humble opinion. Expecting your feedback

@s-alves10 The delete modal is currently a part of ReportActionContextMenu they should have same state for the time being. I think the problem with that is timing right? (we need to make sure events go in a certain order) If this approach is getting too complicated it may be a good idea to take a step back and find another approach.
(Let's not worry about the emoji picker for now)

2. It has context menu open: Let's discuss this point.

@s77rt I was checking the various scenarios here but do not understand the use case when context menu is open. Are the steps to reproduce this mentioned somewhere? Please let me know. Thanks. Sorry if I missed this somewhere.

@rojiphil Edit a message, clear the input then press the save button. This will show the delete modal which is a part of the context menu.

Got it. Thanks. Investigating...

@s77rt
I think we can only one state for ReportActionContextMenu as you mentioned.
We can solve the case 2 as follows
We should show the main composer only if we delete or unmount the active message. A message is considered active if:

It's focused: This is a straightforward check as we have a state that tracks the focus
It has context menu open. We can check if the report action has context menu open using ReportActionContextMenu.isActiveReportAction.
Don't clear the reportAction for ReportActionContextMenu in modalHide. We clear it when an action gets focused. And we set reportAction when context menu is opened or delete modal is opened without clearing it. This way isActiveReportAction returns true when on unmount or deleteDraft call

@s-alves10 Thanks for the update. For the most part I like the idea and the consistency how we handle both the emoji picker and context menu in the same way. Unfortunately we have a problem:

Don't clear the reportAction for ReportActionContextMenu in modalHide.

We can't just undo that because this was added here https://github.com/Expensify/App/pull/20185 to fix https://github.com/Expensify/App/issues/18165. We need to fix the original issue in another way before we can remove that code.

Proposal
Please re-state the problem that we are trying to solve in this issue.
There are focus issues when multiple messages are been edited. Also, there are consistency issues with the show/hide of the Composer when multiple messages are edited. The scope is covered in the mentioned use cases in the problem description which needs to be addressed.
What is the root cause of that problem?
Note: Message Editor refers to the editor of a message which is a report action.
The current implementation hides the Composer when the Message Editor gets focus and shows the Composer whenever the Message Editor is blurred or draft is deleted. The Composer is also shown on unmount of the Message Editor if it is the currently focused one as shown here. The current approach relies on showing/hiding the Composer when the focus comes in and goes out of the Message Editor.
Hiding the Composer when Message Editor gets focused is the right thing to do. Then, the next logical question to ask is 'When do we show the Composer?' We are currently trying to handle this during the unmount here and on blur here. There, we are trying to identify all the cases where we should skip showing the Composer. Now, that would be a challenge as we have to target all individual cases. Further, showing/hiding the Composer will mount/unmount itself and will also bring/lose focus on the Composer. When there are multiple Message Editors and the focus needs to be on one of the Message Editors, this will bring a competition between Composer and Message Editor to gain focus. According to me, these are the problems that we need to address to resolve this problem.
What changes do you think we should make in order to solve the problem?
This proposal is an alternate approach.
The proposed solution is to move the decision of where to put focus at a later point when Composer is mounted or props get updated. Further, instead of identifying all individual cases where we should skip showing the Composer, let us show the composer only when it is needed.
The implementation can be as follows:

We know that there can be only editor (i.e. Composer or Message Editor) which can gain focus. To ensure that the appropriate editor gets focus, let us introduce a callback focusActionItemCallback for Message Editor just like we have focusCallback here for Composer. Further, we can update onComposerFocus, focus and clear to accommodate focusActionItemCallback in ReportActionComposeFocusManager.js. Also, we can add a helper function isCurrentActionItem to identify if the Message Editor is the currently focused one.

To use focusActionItemCallback, let us first call onComposerFocus for the Message Editor when it gains focus here. Additionally, we can clear the callback for Message Editor whenever a) Composer gains focus here and b) Message Editor is unmounted (addressed in point-4). Now, whenever focus gets called, Composer or Message Editor gets focused depending on the current use case.

Now, let us call focus at the required places. First, when the Composer is mounted, we can replace this.focus() here with ReportActionComposeFocusManager.focus(). Also, the same can be done for update here. By making the decision on where to focus at the time of mount/update of Composer, we can avoid competition between Composer and Message Editor for gaining focus. Also, we can remove these lines as deleteDraft will always unmount the Message Editor and we will handle this case in point-4 below.

As mentioned earlier, let us show the composer only when it is needed. Now, we know that the only case when we need to show the composer is if the currently focused Message Editor is been unmounted. We can leverage the recently added helper function isCurrentActionItem for this purpose. Also, when the Message Editor unmounts, we need to bring the focus back to the right place (i.e. Composer or another Message Editor). We can do this by replacing this code with the following:

    useEffect(() => {
        return () => {
            if (ReportActionComposeFocusManager.isCurrentActionItem(onFocusCallback))
            {
                ReportActionComposeFocusManager.clear(true);
                ComposerActions.setShouldShowComposeInput(true);
            }
            ReportActionComposeFocusManager.focus();
        };

    }, []);

Case 2 and Case 3 in the problem statement should get resolved with the above implementation. Regarding Case 1, the keyboardDidHide handler triggers the problem. For me, Case 1 gets fixed by doing just like the web version here. I am not clear why the keyboardDidHide handler was added in the first place. Or may be I am missing some information here due to internal discussions. @s77rt , would you know the issue that will get reproduced if we do not use the keyboardDidHide handler?

What alternative solutions did you explore? (Optional)

@s77rt I have tested the proposal across platforms and it seems to work well. Please do share your thoughts on the proposal. Also, let me know if there are any additional cases that you would like me to test. Thanks.

@rojiphil Thanks for the proposal. If I understood the concept, we will replace the "focused" state management: instead of using the isFocused() method we will be using our own. We will set the flag as focused in the native focus callback but we won't unset it in the native blur callback instead we will unsent it when we focus on another input or delete that input. Would that be about right?
Also the focus callback or focusActionItemCallback is not needed here, right?

If I understood the concept, we will replace the "focused" state management: instead of using the isFocused() method we will be using our own. We will set the flag as focused in the native focus callback but we won't unset it in the native blur callback instead we will unsent it when we focus on another input or delete that input. Would that be about right?
Also the focus callback or focusActionItemCallback is not needed here, right?

@s77rt You are right that we will replace the "focused" state management by using our own. Further, as you mentioned, we will not unset the flag in the native blur callback but we will unset it when we focus on another input or delete that input. However, there is an additional point to take care of. A deleted input may or may not be the currently focused one. We need to know this because we show the Composer only for a deleted input that was currently in focus. A generic flag will not help here. This is where an input-specific focusActionItemCallback will help. We compare focusActionItemCallback with the deleted input's focus callback to determine if this is the one which was in focus (covered in point-4 of the proposal). So, I think, focusActionItemCallback is needed. And, by the way, focusActionItemCallback is also used as a flag here.

@rojiphil Can you please clarify what would we exactly store in focusActionItemCallback?

@s77rt focusActionItemCallback can store a reference to the following callback within ReportActionItemMessageEdit component:
    const onFocusCallback = useRef(
        () => {
            textInputRef.current.focus();
        },
        [textInputRef],
    );

Hope this clarifies. Please let me know if any more information is required.

@rojiphil Can we compare two refs? or what would we compare in isCurrentActionItem?

@s77rt Since useRef returns a reference to Object, focusActionItemCallback and onFocusCallback can be simply compared with === as follows:
function isCurrentActionItem(onFocusCallback) {
    return focusActionItemCallback === onFocusCallback;
}

I don't know if there is a better solution for this. Would be glad to explore if any.

@rojiphil Got it. I think that's a valid approach but it may get too complex if we want to cover all the cases. For instance I think if we open the emoji picker of any message, the isCurrentActionItem would return false because we didn't focus on the input. This would mean if we delete that message the Main Composer won't show up.

@rojiphil Sorry I meant if that message is deleted from another device while emoji picker is still open from your side

I meant if that message is deleted from another device while emoji picker is still open from your side

@s77rt Got it. I tried this scenario too. Note the deletion of messages Hello One and Hello Two from another device. LGTM. The test video is on the device on my side.
https://github.com/Expensify/App/assets/3004659/5bea8c52-f276-4b03-a32f-3a595d828e69

but it may get too complex if we want to cover all the cases.

@s77rt, tbh, I don't think our concept is complex. Actually, we have taken a step or two back and have taken a holistic view to consistently address the show/hide of Composer and manage the focus on multiple message edits and Composer. But, I agree that number of cases is many as it has been already evident through the history of issues related to this topic.

@s77rt
Regarding https://github.com/Expensify/App/issues/23908#issuecomment-1673669023, I suggest to clear active report action below
https://github.com/Expensify/App/blob/21d483c098621b59feb0522475d75ceb13e4e2a8/src/pages/home/ReportScreen.js#L174
        ReportActionContextMenu.clearActiveReportAction();

I think this is reasonable in the sense that it reset the context menu when open a report. Let me know your thoughts

@s77rt Here are a few more test videos with the iOS simulator. Updates from Onyx due to remote delete of inputs will trigger either unmount or blur events locally on inputs, and depending on the use case the Main Composer is shown.

Remote Deletion Test 1
https://github.com/Expensify/App/assets/3004659/62801b51-88be-417e-bde8-adc3ad8f0ba4

Remote Deletion Test 2
https://github.com/Expensify/App/assets/3004659/7cda2b27-a7d2-41c5-b1ac-2502c7267cfd

I can also share a test version on Git Hub if that helps in reviewing the concept in more detail. Please let me know.

@techievivek  @s77rt A humble request. If possible, can we increase the bounty for this issue as the number of surrounding cases to take care of is more than the listed ones in this issue (e.g. deleting from the remote device)? And, especially, if we are taking a step back and taking a holistic view as mentioned here, I think there is even a stronger case for the increase in bounty. What are your thoughts?

Proposal
Updated

@s-alves10 Thanks for the update. I think the alternative solution for https://github.com/Expensify/App/issues/18165 is a workaround, changing the report have nothing to do with the active context menu.

@s77rt
Yeah. That's not a solution. It'll cause a regression
In order to solve https://github.com/Expensify/App/issues/24016, I think removing the clear action in modalDidHide is unavoidable. It would be good if we can clear the active report action when the report action is unmounted, but this isn't the solution for https://github.com/Expensify/App/issues/18165 because we use the stack navigator and unmounting doesn't happen. So we need another solution for doing this.
I suggest the creation of isContextMenuVisible() function. We have isPopoverVisible state already. We highlight the report action only when hovered or context menu is visible. I think this is reasonable and tested well.
Let me know your thoughts

When we trigger the unmount, we will check if isCurrentActionItem but that's supposed to be false because the input was never focused, so the main composer won't show up.
For the case you demonstrated the composer is shown because it was already visible, it seems to show up when you press the emoji picker button so the bug is not reproducible but I guess this is only the case for native, on mWeb I think the bug may be actually reproducible.

@s77rt Thanks for the feedback. LGTM on mWeb too. Attaching test videos for the same. Agree that isCurrentActionItem will not be true in the demonstrated case and Main Composer will not be shown due to this. However, the ReportActionComposeFocusManager.focus() added here will try to focus on the input but will be blurred due to the presence of emojipicker/context-menu. This ensures that the Main Composer is shown. This is consistent behavior across native/mWeb.

Emoji Picker Test
https://github.com/Expensify/App/assets/3004659/2402c011-4caa-42cb-b8ea-1d3452c93ca0

 Context Menu Test
https://github.com/Expensify/App/assets/3004659/6203ff90-be44-41cf-b962-78818b1224e0

@rojiphil Why do we have the ReportActionComposeFocusManager.focus() outside the if condition?

Proposal
Updated
@s77rt
isContextMenuVisible isn't needed. We need isDeleteConfirmModalVisible function. I updated the proposal accordingly

@rojiphil Why do we have the ReportActionComposeFocusManager.focus() outside the if condition?

@s77rt That's a good question. We added this outside the if condition because where the focus should be is independent of the message input being unmounted. Fundamentally, we keep our two objectives (i.e. show/hide of Main Composer and the focus management) separate and address them.

@s-alves10 Thanks for the update. I think isDeleteConfirmModalVisible is still not needed. isActiveReportAction is the only function that should be used. Otherwise we would be just adding another condition to identify when a message is active (the 3 mentioned here and this "It has delete confirm modal visible".

@rojiphil It's still not clear, why we are trying to focus the Main Composer when any of the messages is deleted? Seems like a workaround.

@s77rt Sorry for not being very clear with my explanation. Let me explain. As mentioned in point 1 in proposal here, focusActionItemCallback is also integrated into ReportActionComposeFocusManager.focus() to have centralized focus management. Further, as mentioned in point 4 of proposal here, we call ReportActionComposeFocusManager.focus() to ensure that focus remains with another focused message input when an explicit mouse click has resulted in the unmount of this unfocused message input. This is the reason why ReportActionComposeFocusManager.focus() is outside the if condition. Hope my explanation is better now. Please let me know.

Proposal
Updated
@s77rt
You're right. I've found a solution. We can change the confirm callback of the showDeleteModal. Please check the updated proposal

@rojiphil That's still looking as a workaround. There is no real reason to call ReportActionComposeFocusManager.focus() when any message is deleted.
Any alternative solution for fixing this without introducing a new focus management?

@s-alves10 I don't see what changes we are making to showDeleteModal. We were already using the isFocusedRef that way here https://github.com/Expensify/App/pull/23618. Also the isFocusedRef is available without the need to pass it as a parameter.

@s77rt
Yes. That's the point. The value of isFocusedRef.current is different when we click save button and we click delete button. RIght? I wanted to use its value at the time we click save button.

Proposal
Updated
@s77rt I get your point. Have updated the proposal to remove the new focus management and instead use isActiveReportAction. Please refer the section of Alternative Solutions for the details. Thanks.

Proposal
Please re-state the problem that we are trying to solve in this issue.
Main composer displayed editing two messages
What is the root cause of that problem?
When ReportActionItemMessageEdit.js is unmounted and deleteDraft is called, we will set setShouldShowComposeInput to true. This action makes the main composer visible regardless of the status of other editable composers.
What changes do you think we should make in order to solve the problem?

Instead of setting the value of shouldShowComposeInput we can add a counter above and When we set the value to 'false', we'll increase the counter. And when we set it to 'true', we'll decrease the counter. We'll only set shouldShowComposeInput to 'true' when the counter is at 0. This way, we'll make sure that the compose input is shown only when no other ReportActionItem is being edited.
To avoid the unreliability of using a number counter we can track how many active ReportActionitems are being edited by making the counter an array and push the unique index of the ReportActionItem that is being edited to it. using the length of the array we will check if it is empty or not, empty means there is no item being edited.
For example, let's say we have a list of editItems with numbers [index1, index2]. This means we're currently working on editing two items in ReportAction. When we delete a draft, we'll remove it from the list using its matching index. this will make sure, the primary composer will remain hidden until any ongoing edits of ReportAction items are canceled.
https://github.com/Expensify/App/assets/75031127/dc162797-f165-4539-b5eb-3c0708aba64e
What alternative solutions did you explore? (Optional)
N/A
cc @s77rt

@s77rt
In order to use the value at the time we click save button, we use this type of call: () => deleteDraft(isFocusedRef.current). If we don't use parameter and use the isFocusedRef.current inside the deleteDraft function, this is not the case. But we use pass by value here in this syntax.

@getusha Thanks for the proposal. Your RCA is correct. I think the solution may not work if we open a report that already has some report actions in edit mode as the counter at this point would be zero.

@s77rt that's a valid case, here after implementing the solution.
https://github.com/Expensify/App/assets/75031127/be2122e5-d3b4-460d-947d-1df504ae886a

Proposal
Updated
Here is the demonstration code
  const isFocusedRef = { current: false };
  const callback = (() => { const focused = isFocusedRef.current; return () => console.log(focused); })()
  setTimeout(callback, 1000);
  isFocusedRef.current = true;

@getusha
If there is no focused draft, the main composer should be shown, I think

@s-alves10 my solution is also flexible to handle that case.
https://github.com/Expensify/App/assets/75031127/bc08a58d-a4ca-4a63-bcd2-822932761e76

@s-alves10 Oh I got it! Indeed that's a good solution and we can have it without that weird function wrapper, we can just pass the isFocused (from state and not from the ref): () => deleteDraft(isFocused). However unfortunately this won't work if the message is deleted from another device while you have the delete modal shown up because the logic in the useEffect is still not taking the context menu in consideration.

@s77rt
Yeah. && !ReportActionContextMenu.isActiveReportAction() should be added to the if condition, I think

this solution won't work as intended because ReportActionContextMenu.isActiveReportAction won't be true as you'd except and this is due to the changes made by this PR #20185.

@s77rt
If I understand you correctly, your concern is that since reportAction is reset here during onHide, ReportActionContextMenu.isActiveReportAction will not return true. But, I am confident that it will return true. The reason is that ContextMenu and DeleteModal are hidden when the parent component ReportActionItem is destroyed as shown here.  As such, the unmount of ReportActionItemMessageEdit will be called first. At this time,  the reportAction will not be lost.
As mentioned in my proposal, the only thing that is required is to show the Main Composer if there are EmojiPicker/ContextMenu shown for the deleted message input. No need to check for any other flags or even hide the emojipicker/contextmenu. Please let me know your thoughts.

@getusha I think it's better to avoid counters here. There may be more report actions in edit mode than you can see. Also I don't think counters will fix case 2 outlined above.

@s-alves10 Any alternative solution for https://github.com/Expensify/App/issues/18165? That should probably help us a ton

@s77rt is case 2 the following?

Case 2:
Send two messages (message A and message B)
Switch to edit mode in message A
Switch to edit mode in message B
Click the cancel button of message A
Main Composer is shown but it shouldn't because focus is still on message B

@s77rt
I've found a better solution. We can clear it in confirm/cancel callback here
https://github.com/Expensify/App/blob/402c70c7b5fe5b0f7ee7bb656d112d0e003ebe0e/src/pages/home/report/ContextMenu/ContextMenuActions.js#L306-L315
There are only two calls: above call and the call in ReportActionItemMessageEdit

@rojiphil

If I understand you correctly, your concern is that since reportAction is reset here during onHide, ReportActionContextMenu.isActiveReportAction will not return true

Yes, that's correct.

The reason is that ContextMenu and DeleteModal are hidden when the parent component ReportActionItem is destroyed as shown here

I don't think that's relevant, the modal is also hidden if you just press the confirm button.

As such, the unmount of ReportActionItemMessageEdit will be called first

Interestingly this is not true if the message is deleted from another device. But as mentioned in the above point, this may actually be unrelated.

@getusha Yes

@s77rt
In the call in ReportActionItemMessageEdit, we don't clear report action in callbacks.

@s77rt
Further to my comment here and your reply here. My comments were specifically for the case of deletion of message input from another device. I also tested this in the test video on mWeb as shown below. Isn't this the test case? Or am I missing something here?  Please let me know.
https://github.com/Expensify/App/assets/3004659/53a4955e-75c0-4fd0-8f88-29703b3d8f83

@s-alves10 It's not clear, can you clarify when to clear the report action exactly?

the unmount of ReportActionItemMessageEdit will be called first. At this time, the reportAction will not be lost.

@rojiphil Are you sure?

Are you sure?

@s77rt @s-alves10 Have just sent a test video here. Isn't this the test case? If not, share with me the specific test case and I will verify it again.

@s77rt
We set empty functions as confirm/cancel callbacks in showDeleteModal call in context menu now. We can set confirm/cancel callback there like () =>ReportActionContextMenu.clearActiveReportAction(). Context menus doesn't work for messages in edit mode.
Change nothing in showDeleteModal call in ReportActionItemMessageEdit
Clear active report action in focus event as I proposed earlier

@rojiphil https://github.com/Expensify/App/issues/24016

Main Composer is shown but it shouldn't because focus is still on message B

@s77rt will the focus remain on message B? Could you please provide me with a video? can be from staging or prod.

This issue has a lot of edge cases. Hoping all contributors read all history of this issue and https://github.com/Expensify/App/issues/17531, https://github.com/Expensify/App/issues/24016

@s-alves10

We can set confirm/cancel callback there like () =>ReportActionContextMenu.clearActiveReportAction().

Is that meant to be a solution for https://github.com/Expensify/App/issues/18165?

Clear active report action in focus event in ReportActionItemMessageEdit

We should clear the report action when the context menu is hidden as that's the natural way and not based on another focus event

@rojiphil #24016

@s77rt Thanks for sharing that. The issue you mentioned above is exactly the one I tested and sent the video here. Just curious, why do you think this should not work?

@s77rt

Is that meant to be a solution for #18165?

yes

We should clear the report action when the context menu is hidden as that's the natural way

What did you expect from https://github.com/Expensify/App/issues/23908#issuecomment-1684261955?
In addition, if we clear report action in modal hide, race condition can appear as in EmojiPicker, I think

@rojiphil #24016

@s77rt Extremely sorry. Here, you were not referring to remote delete but local delete. Let me check this.

@s77rt incase you missed my comment https://github.com/Expensify/App/issues/23908#issuecomment-1684305387

@getusha Yes the focus should remain on message B. You can apply this PR https://github.com/Expensify/App/pull/23618 to see the wanted results for that case.

@s-alves10

Clear active report action in focus event in ReportActionItemMessageEdit

Do we need to do that? What if we kept the value untouched same as done in the emoji picker? (we don't change the report action in focus event for the emoji picker right?)

@s77rt

Interestingly this is not true if the message is deleted from another device.

The test video here  demonstrates that it works the way I expected i.e. unmount of ReportActionItemMessageEdit first, then, ReportActionItem unmount is called.

I don't think that's relevant, the modal is also hidden if you just press the confirm button.

If we press the confirm button, the focus is already on the message input. So, the logic in unmount of focused message input will ensure that the composer is shown.

@rojiphil

unmount of ReportActionItemMessageEdit first, then, ReportActionItem unmount is called.

I tested this with console.log and ReportActionItem is actually the first. But regardless we need to fix the case where we delete the message locally as well.

@s77rt i see the expected result, it can be applicable to my proposal.
here is the result:
https://github.com/Expensify/App/assets/75031127/3f903035-f88b-4792-9eb3-a4ea700cf3c5

@getusha Can we please avoid using a counter? As mentioned earlier this may cause an issue when dealing with old report actions that are left in edit mode.

@s77rt which means we only want to handle this case for Report actions which are most recent?

I tested this with console.log and ReportActionItem is actually the first. But regardless we need to fix the case where we delete the message locally as well.

@s77rt You are right. ReportActionItem is actually the first. Sorry about that.
Here are my findings. The reason why ContextMenu will work in my proposal is this:
In the case of ContextMenu, whether it is Remote Deletion or Local Deletion, the focusActionItemCallback will refer to this message input. So, the unmount logic of ReportActionItemMessageEdit will kick in to show the Main Composer as mentioned in point(4) of my proposal
Does this make sense now?

@getusha

which means we only want to handle this case for Report actions which are most recent?

No, the solution should not relay on other report actions at all.

i would love if you could explain what issue exactly causes

Send two messages (message A and message B)
Switch to edit mode in message A (counter = 1)
Switch to edit mode in message B (counter = 2)
Delete message B from another device (counter = 1)
The Main Composer should show up but it won't because the counter is not zero

@s77rt

Do we need to do that? What if we kept the value untouched same as done in the emoji picker? (we don't change the report action in focus event for the emoji picker right?)

We need to clear active action for emoji picker as well. We told that before

@rojiphil Are you referring to ReportActionComposeFocusManager.focus();? That's a failsafe call. I don't think it's really needed (outside the if condition). It's only there as a workaround - we shouldn't fall into that fail scenario.

@s-alves10 Ah for case 3, right sorry about that.

Instead of clear active action in modal hide, we can clear it in confirm/cancel callback here

How is that different? onModalHide we will still call callbackWhenDeleteModalHide which will call our callback.

@s77rt
Yes. So if we don't clear active action in the callback, active action won't be cleared. right?
We need to clear active action for context menu's delete modal. And we shouldn't clear active action for delete modal in publishDraft of ReportActionItemMessageEdit

@s-alves10 I'm not sure what we are trying to solve here? This is supposed to be another fix for https://github.com/Expensify/App/issues/18165 (and we revert the linked PR) But that solution is pretty much the same and won't do us any good as ReportActionContextMenu.isActiveReportAction would still return false.

Send two messages (message A and message B)
Switch to edit mode in message A (counter = 1)
Switch to edit mode in message B (counter = 2)
Delete message B from another device (counter = 1)
The Main Composer should show up but it won't because the counter is not zero

@s77rt if you see the places where we call ComposerActions.setShouldShowComposeInput(true); you can see it's also called when the component is unmounted right? which includes when the report item is deleted. please let me know if there are other test cases i should test against.
https://github.com/Expensify/App/assets/75031127/94b517ec-6e7c-48c5-91df-a7fccac05896

@getusha In your attached video the main composer did show before we get to switch to the other message. Try to focus again on the message.

Focus on message A (counter = 1)
Focus on message B (counter = 2)
Delete message B from another device (counter = 1)
The Main Composer should show up but it won't because the counter is not zero

@s77rt when the focus is removed the counter will be decreased you are aware of that right?

@getusha It's not the case when the focus is transferred to another message

@getusha It's not the case when the focus is transferred to another message

@s77rt  In order to focus Message B, message A will be blurred eventually, when that happens the counter will decrease.
https://github.com/Expensify/App/assets/75031127/5b0cee6f-cccf-4bf3-ba35-9c3fce0bfc9f
let me know if i did something wrong while testing.

@getusha I'm not sure how this is working. It does not go with your proposal. Why is the counter in this case zero (or why it's working)?

message A will be blurred eventually, when that happens the counter will decrease

According to onBlur logic we will only call openReportActionComposeViewWhenClosingMessageEdit if the next focused element is not another message input, thus we wouldn't decrease the counter in this case.

@s77rt

I'm not sure what we are trying to solve here? This is supposed to be another fix for #18165 (and we revert the linked PR) But that solution is pretty much the same and won't do us any good as ReportActionContextMenu.isActiveReportAction would still return false.

What I want to do here is to remove the clearing action in modalDidHide. What is important is that we can control whether clear or not the active action. If we don't clear the active action in confirm/cancel callback, ReportActionContextMenu.isActiveReportAction would return true.

@getusha I'm not sure how this is working. It does not go with your proposal. Why is the counter in this case zero (or why it's working)?

@s77rt I can share the diff where i call the functions if you are interested 😄

Proposal
Updated
@s77rt

Are you referring to ReportActionComposeFocusManager.focus();?

I am not referring to ReportActionComposeFocusManager.focus(); for solving the ContextMenu issue.
Please do have a look at the updated Alternative Solutions section in the proposal. I have attempted to bring more clarity to the changes required.
Also, please let me know your thoughts.
Extract from the Proposal
Alternative Solution to new focus Management:
Purpose:
a) Remove the usage of ReportActionComposeFocusManager.focus() from the main proposal as there is no real reason to call this. Reference comment here
b) Solve the edge case in mWeb i.e. Main Composer does not show up when EMojiPicker is already displayed for an unfocused message input and the same message input is deleted from the remote device. Reference comment here
Note: Usage of isCurrentActionItem is still very much there as mentioned in the main proposal.

@s-alves10

If we don't clear the active action in confirm/cancel callback, ReportActionContextMenu.isActiveReportAction would return true

Right, but we have to clear it, otherwise we would reintroduce https://github.com/Expensify/App/issues/18165 or is there a condition to decide whether to clear it or not?

@getusha

I can share the diff

Code diffs are discouraged. Can you please explain the raised concern in plain English?

@rojiphil Can we please avoid adding a new focus management. I think we already have the info we need, we just need to manage them well. We have the focus info about the input itself, its emoji picker and its context menu.

@s77rt
As I said before, we clear that in context menu delete. This will prevent https://github.com/Expensify/App/issues/18165
And we don't clear that in publishDraft. This makes the ReportActionContextMenu.isActiveReportAction return true

@s77rt
We need to clear the active action on unmount before showing main composer
            if (ReportActionContextMenu.isActiveReportAction(props.action.reportActionID)) {
                ReportActionContextMenu.clearActiveReportAction();
            }
            if (EmojiPickerAction.isActiveReportAction(props.action.reportActionID)) {
                EmojiPickerAction.clearActiveReportAction();
            }

That is, if a message in edit mode is an active action for ReportActionContextMenu or EmojiPickerAction, we should clear active action when the draft is unmounted

@s-alves10 This still does not completely solve the regression
https://github.com/Expensify/App/assets/16493223/8d849fd6-bb9b-456d-9968-8ee5e57e7f93

@s77rt
It works fine for me. I don't understand how the above regression happens. Will you explain the reason for the above regression in words?

@s-alves10 After we clear the report action, the report action won't re-render and the highlight is still there (if you navigate away and back the highlight won't be there). Please notice the difference between the two videos I sent (those are 2 cases that are a bit different).

Code diffs are discouraged. Can you please explain the raised concern in plain English?

According to onBlur logic we will only call openReportActionComposeViewWhenClosingMessageEdit if the next focused element is not another message input, thus we wouldn't decrease the counter in this case.

@s77rt the function to remove/decrease the counter will be called right below/above,  setIsFocused(false) since the checks are not relevant to the action we are performing here.

@s77rt
Yeah. I've tested those 2 cases and works fine.
Whenever the active report action in edit mode unmounts, we clear the active action. So when you cancel the message in edit mode, active is cleared and can't be highlighted.

@getusha That would work I suppose but I still don't think using a counter is the best approach here. Ideally we won't need any new info to store.

@s-alves10 Can you please test against main? If possible also include a video for the two cases, perhaps I'm missing something.

@getusha That would work I suppose but I still don't think using a counter is the best approach here. Ideally we won't need any new info to store.

@s77rt May i ask the reason why we don't want to store any new info? comparing it to other proposals which are more over complicated for this issue, will using a counter bring any issues? i appreciate your input. 🙇

@s77rt
Yes. I applied the changes from the current main branch and it did cause a regression mentioned here
I suggest to clear the report action in modalDidHide as it is now and use the () => deleteDraft(isFocused)

Proposal
Please re-state the problem that we are trying to solve in this issue.
Chat the Main composer is displayed when editing two messages
What is the root cause of that problem?
With recent changes from this PR, the root cause might change. It's now caused by clearing the editcomposer callback here:
https://github.com/Expensify/App/blob/4f412db5e26782740529eb0ef17b5db157628413/src/pages/home/report/ReportActionItemMessageEdit.js#L215
What changes do you think we should make in order to solve the problem?
I think we can remove that line and the original issue from https://github.com/Expensify/App/issues/23898 and cases in this issue will be resolved. We restore the cursor to last focused input instead of main composer

Proposal
Please re-state the problem that we are trying to solve in this issue.
Chat the Main composer is displayed when editing two messages
What is the root cause of that problem?
With recent changes from this PR, the root cause might change. It's now caused by clearing the editcomposer callback here:
https://github.com/Expensify/App/blob/4f412db5e26782740529eb0ef17b5db157628413/src/pages/home/report/ReportActionItemMessageEdit.js#L215
What changes do you think we should make in order to solve the problem?
I think we can remove that line and the original issue from https://github.com/Expensify/App/issues/23898 and cases in this issue will be resolved. We restore the cursor to last focused input instead of main composer

@getusha

May i ask the reason why we don't want to store any new info?

I think we have enough info to determine whether the composer should be shown or not.

@s-alves10

I suggest to clear the report action in modalDidHide as it is now and use the () => deleteDraft(isFocused)

I think we discussed this earlier. This won't work if the message is deleted from another device. We can't have the logic restricted to deleteDraft function, we need to cover the useEffect callback as well.

@wildan-m Thanks for the proposal. That PR seems to have some impacts on this issue indeed. Thanks for linking! But the root cause is still not clear. Can you please elaborate?
Please note that the use of _.defer or setTimeout is strictly discouraged as they are workarounds.

@s77rt will use InteractionManager.runAfterInteractions is also discouraged?

@s77rt proposal updated with focusWithDelay option to make sure we perform focus when ComposerFocusManager.isReadyToFocus is true

@rojiphil Can we please avoid adding a new focus management. I think we already have the info we need, we just need to manage them well. We have the focus info about the input itself, its emoji picker and its context menu.

@s77rt
I am not suggesting to use the new focus management. What I am simply suggesting is to replace the current isFocusedRef with another one (say currentFocusedMessageEdit). isFocussedRef is just an indicator of whether this message input has focus or is blurred and is not fully within our control. However, what we really need to know during unMount is if this message input was where the focus should have been. If we know this, I don't think we have to worry about temporary UI elements (like context menu, emoji picker, and any other UI that are introduced in the future) being displayed at the time of deletion of the draft. For example, when the focus is on message input A and its deletion occurs via Context Menu's Delete Dialog, isFocusedRef will return false but currentFocusedMessageEdit will return true. We can use this information to show the Main Composer. In this case, we do not need to think about depending on active ReportAction of the context menu.
Also, if focusActionItemCallback is causing confusion here, we can replace this with currentFocusedMessageEdit which can refer to the textInputRef of ReportActionItemMessageEdit.
The only exception I noticed in my proposal is an edge case in mWeb as per your comment here. But, this can be fixed by showing Main Composer during Unmount when the following conditions are met: i.e. a) EmojiPicker is visible, b) EmojiPicker's active action is for this message input, and c) This is a mobile browser.
Managing with existing values of isFocusedRef and active ReportAction of emojipicker/context menu is already a challenge due to the risk of regressions. Compared to this, isn't my proposal simple and straightforward?
Please let me know your thoughts.

Proposal
Updated

I think we have enough info to determine whether the composer should be shown or not.

@s77rt you probably are correct, i am just comparing the solution to existing proposals here, which utilize a bunch of new info including the Keyboard.

@wildan-m The use of InteractionManager.runAfterInteractions is also discouraged as it uses setTimeout internally. For case 2, I think your solution is easier but does not work if we are working with 3+ messages
https://github.com/Expensify/App/assets/16493223/8273cf5a-9467-4dba-a68f-13f45e9a5521

@rojiphil I understand your solution but we are essentially creating a new focus management because we will have two sources that tell us whether the input is focused or not: isFocused (managed natively) and currentFocusedMessageEdit (managed manually i.e. we populate this variable).

@s-alves10 Thanks for the update. The solution for https://github.com/Expensify/App/issues/23908#issuecomment-1687155281 looks promising, but I see another problem (unrelated to this issue) which is that isContextMenuActive is a state. I don't think a state is needed here, can we just have isContextMenuActive = ReportActionContextMenu.isActiveReportAction?

There is another issue mentioned here.

This seems like the wrong url, can you please double check?

Proposal
Updated
@s77rt
I'm sorry. Wrong URL was updated
Regarding isContextMenuActive, I think we need to have this as state because dynamic update should re-render the component. For example, when toggleContextMenuFromActiveReportAction is called, we need to re-render the component. Please let me know your thought

Proposal
Updated
@s77rt
The proposal is updated with Solution 1 under What changes....  This solution attempts to resolve the problem by remaining within the scope of ReportActionItemMessageEdit.js. Along with isFocusedRef, this approach relies on states for ContextMenu and EmojiPicker which can be appropriately set/reset when context menu/Emoji Picker are shown/hidden.
More details on implementation are included within the proposal under heading Solution 1.
Please let me know if this is fine.

@s-alves10 I think we should not worry about the state use in that function for now. Same thing goes for the other reported slack issue. Can you please just double check that all the 3 cases are fixed in both mWeb and Native and works well if message is deleted either locally or remotely.

@rojiphil Thanks for the update. I don't think we need states for ContextMenu and EmojiPicker because we already have global refs for those and we have functions that return whether a given report action is active.

This is a tricky one and may require some more time to come up with a proper fix(context here: https://github.com/Expensify/App/issues/23908#issuecomment-1662210868).

@s-alves10 can you give us an update when you get a chance? Thanks.

@s77rt
These are the test steps I've done
Note: Step 5 - 6 are for mobile platforms.

Open ND and log in with any account
Open any chat(chat C) and send two messages(message A and message B)
Switch message A to edit mode
Switch message B to edit mode
Hide keyboard(press back button)
Verify that the main composer isn't shown
Click the cancel button of message A
Verify that the main composer isn't shown
Switch message A to edit mode again
Open EmojiPicker on message A and close it
Click message B to focus on it
Click the cancel button of message A
Verify that the main composer isn't shown
Delete all contents of message B and press Enter or click Save
Confirm delete modal appears and click cancel
Verify that the main composer isn't shown
Again press Enter or click Save
Click delete on confirm delete modal
Verify that the main composer is shown
Switch message A to edit mode
Click Save button and click cancel on confirm delete modal
Switch to another chat and return back to chat C
Click cancel in message A
Verify that the message A isn't highlighted

The solution looks working fine.
I think it would be good to double the bounty. This issue has a lot of edge cases

@s-alves10

Again press Enter or click Save
Click delete on confirm delete modal
Verify that the main composer is shown

Can you please explain how is this working? The main composer is not showing due to some recent PR that got merged and reintroduced https://github.com/Expensify/App/issues/24016

@s77rt
Yes, when press Enter, the issue appears. I explained about this
The root cause of this issue: when deleting message, after the message gets focused and the unmount handler is called in a quick succession after deleteDraft is called. When get focused, setIsFocused(true) is called but isFocusedRef.current value doesn't change until this effect callback is called. The unmount handler is called just before this ref value is updated and so the handler returns here.
In order to solve this issue, we need to sync the isFocused and isFocusedRef.current. So remove the above useEffect and set isFocusedRef.current whenever setIsFocused is called

@s-alves10 Any idea which PR broke this?

The unmount handler is called just before this ref value is updated and so the handler returns here.

I don't think this is the case. The useEffect callbacks are called in order. isFocusedRef is always updated first before executing the cleanup callback.

@s77rt
I verified this by logging
Yes, useEffect callback is called in order. but the cleanup function of useEffect callback is called before any effect callback is called.

This is a tricky one

@techievivek @s77rt, Where do we draw the line when it comes to introducing two local states and solving the problem in a straightforward manner vis-a-vis trying to reuse global refs and trying hard to avoid regressions due to global usage of these refs? Curious to learn if I am missing something here regarding best practices followed at Expensify. For context, please compare Solution 1 in the proposal here and Case 2 and Case 3 Solution in the proposal here.

@rojiphil There should be only one source of truth.

@s77rt
Please check https://legacy.reactjs.org/docs/hooks-reference.html#cleaning-up-an-effect
After the cleanup is called, effect callback isn't called because the cleanup is unmount in our case

@s-alves10 How is that relevant? We are not interested in the effect of the second callback. We have the following:

callback-1
callback-2
cleanup-callback-2

callback-1 is always executed before cleanup-callback-2. Thus isFocusedRef is updated correctly (in callback-1).
callback-2 is irrelevant here.

@s77rt
You can check that isFocusedRef value in the cleanup function

@s-alves10 Ah so the state is not getting updated since the update is async and we happen to unmount the component before setting the new state.

@s-alves10 OK I got it it but it's not clear how this suddenly broke. Any idea?

@s77rt
I've checked the old version(18th Jul) and there was already this issue. This is an old issue, not introduced recently

@s-alves10 I don't know for sure but something is different, this was not a bug. It was introduced once here https://github.com/Expensify/App/issues/24016 then after we revered the offending PR the bug was no longer reproducible. Now it's suddenly reproducible again.

@s77rt
https://github.com/Expensify/App/issues/24016 was the issue that happens when click send button. This works fine now. The current issue happens when press Enter key. They are different

@s-alves10 I have been checking this and the root cause seems related to the Main Composer showing up when pressing Enter due to the call of openReportActionComposeViewWhenClosingMessageEdit in onBlur. This function is not called when pressing the save button because it's excluded (saveButtonID).

I think this issue is complex enough. The newly reported issue should be handled separately unless it's a blocker which does not seem to be the case, right? cc @s-alves10

@s77rt
I think so, but I'm afraid the new issue would be a regression of the PR as before.

@s77rt
Yeah. You're right. The new issue can't be a regression. I agree

@s-alves10 Ok great. In this case let's proceed and fix the current reported bugs for this issue.
🎀 👀 🎀 C+ reviewed
Link to proposal

hmm unassigning myself since an internal engineering is already involved and i assume @techievivek has way more context here.

@techievivek can you help move this one along?

Sure, I can handle this one.

Great job on the proposal, @s-alves10! And thanks, @s77rt, for helping move this forward and for handling numerous conversations. 🙇

QA was still able to reproduce this issue

@thienlnam, can you share a video?

@thienlnam
There are 3 cases in this issue this PR solved. Which case wasn't solved?
Please share the reproduction steps

The PR that introduced the bug has been identified:

Case 1: https://github.com/Expensify/App/pull/15201
Case 2: https://github.com/Expensify/App/pull/2320
Case 3: https://github.com/Expensify/App/pull/22984

The offending PR has been commented on:

Case 1: https://github.com/Expensify/App/pull/15201#discussion_r1333198896
Case 2: https://github.com/Expensify/App/pull/2320#discussion_r1333199203
Case 3: https://github.com/Expensify/App/pull/22984#discussion_r1333199524

A discussion in #expensify-bugs has been started: I don't think this is needed. Those bugs could have been caught while testing.
Determine if we should create a regression test for this bug: I think it may be an overkill to add a regression tests for such cases. So nothing to add here.

@kavimuru Do you have any additional details on being able to reproduce this one?

Gentle bump @kavimuru ^

@thienlnam @techievivek
Can I have a status update on this issue?

@thienlnam @techievivek
Can I have a status update on this issue?

We can move forward with the payment here. CC @garrettmknight can we pay the contributors here since the regression period is over, thanks.

Should the payment here be $1000 because this issue is coming from https://github.com/Expensify/App/issues/17531 where we had a regression there

Good catch @s77rt - to recap, we didn't pay out the previous issue because we rolled the PR back, opened up this issue with a $2k start price and since this was created as part of a regression, we'll pay 50%.
Summmary of payments for this issue:

urgency bonus: n/a
regression penalty: yes
Reporter: N/A - original issue was reported by Applause
Contributor: @s-alves10 paid $1000 via Upwork
Reviewer: @s77rt paid $1000 via Upwork

@garrettmknight
I'm not sure why this should be considered as a simple regression of https://github.com/Expensify/App/issues/17531
This solved new issues like https://expensify.slack.com/archives/C049HHMV9SM/p1692747837798019
I hope you explain why
Thanks

I think you can consider this like an extension of https://github.com/Expensify/App/issues/17531. If the automation had allowed, we would have reopened that issue, had you fix the regression and paid out at 50% for the regression. If that had happened and we'd separately focused this issue on the other cases (and no regressions were reported) we'd have paid out $1000 as standard at the time. With that in mind, I could see paying $1500 total out for this issue.

Added $500 to both contracts.


[ Mobile / Desktop ] Create SfPagination

Just adjust designs of the alpaca one leaving business logic untouched.
https://alpaca-storybook-ebth12xpx.now.sh/components/?path=/story/modules-pager--default
https://github.com/SnowdogApps/alpaca-storybook/tree/develop/components/src/03-modules/pager
Also read Working with Alpaca
Can be found on designs on category page

Have you already tried to import some component from @snowdog/alpaca-components?
I couldn't import it at all, maybe I'm doing something wrong but don't know what.
Error with @snowdog/alpaca-components/dist/Pager.vue:
Module not found: Error: Can't resolve '../../02-elements/button/Button.vue'

With @snowdog/alpaca-components or even @snowdog/alpaca-components/Pager.vue (just to give a try):
Module not found: Error: Can't resolve '@snowdog/alpaca-components'

I guess it's just a problem with their local pathes. Please make an issue in alpaca repo and in the meantime just copy the files to SfPagination component folder in some temporary subdir until they'll fix

my failure actually, the package was outdated, 0.1.x-alpha on package.json, and now it's 0.3.x-alpha

Missing more unit tests for the component, mainly for props.


ASP.NET Core 2.0 new features

[ ]  IHostedService to run reporting tasks
[x] IWebhostBuilder to use default web configuration

Fixed as part of formatting/reporting changes https://github.com/AppMetrics/Reporting/commit/254e1542892888ecb9dda78afa5de352ed035ddd


Exception thrown on Speaker adaptation parameter

I got the following error when i enable the fine tuning parameter in hyparams. i have trained the 70k steps on LJ data and now i want to do my speaker adaption. following exception is thrown when i run training:
Initialized Tacotron model.
Tacotron Parameters       29.016 Million.
Traceback (most recent call last):
File "train.py", line 138, in
main()
File "train.py", line 132, in main
train(args, log_dir, hparams)
File "train.py", line 52, in train
checkpoint = tacotron_train(args, log_dir, hparams)
File "/home/////Tacotran_2/Tacotron-2/tacotron/train.py", line 375, in tacotron_train
return train(log_dir, args, hparams)
File "/home/////Tacotran_2/Tacotron-2/tacotron/train.py", line 156, in train
model, stats = model_train_mode(args, feeder, hparams, global_step)
File "//////Tacotran_2/Tacotron-2/tacotron/train.py", line 93, in model_train_mode
model.add_optimizer(global_step)
File "//////Tacotran_2/Tacotron-2/tacotron/models/tacotron.py", line 390, in add_optimizer
update_vars = [v for v in self.all_vars if not ('inputs_embedding' in v or 'encoder_' in v)] if hp.tacotron_fine_tuning else None
File "//////Tacotran_2/Tacotron-2/tacotron/models/tacotron.py", line 390, in
update_vars = [v for v in self.all_vars if not ('inputs_embedding' in v or 'encoder_' in v)] if hp.tacotron_fine_tuning else None
TypeError: argument of type 'Variable' is not iterable
I set only following to true from false:
#Speaker adaptation parameters
tacotron_fine_tuning = True, #Set to True to freeze encoder and only keep training pretrained decoder. Used for speaker adaptation with small data.
###########################################################################################################################################

Thanks for sharing, yes exception is fixed.
But i have an issue that when i run with tacotron_fine_tuning = True, the wavenet model training started with negative loss and its not giving good results, very very poor results.
please suggest.


Async filter function (to interface with database)

This is from tracker's point of view.
I am attempting to extend the usage of filter function by e.g. using a database to query whether the torrent has valid infoHash or not, as well as any other params that needs to be checked.
Database queries, as being network requests, are async in nature, and they don't return a value but a callback to do further processing.
Filter function requires a return value (a boolean).
Is there any way that you can think of to easily do both? As I can see it, there's no way to do this cleanly. Using async module or whatnot isn't going to cut it as it's not the order of the processing, but rather inability to return a value from a callback to original caller (which doesn't make sense anyway).

You're correct, this isn't possible right now since filter has to be a sync function. I know that @astro was using this module to interface with a database.
@astro - any ideas about how to accomplish this?
If we must, I suppose we can make the getSwarm function asynchronous, but I'd like to avoid that if we can.

I've in meanwhile used a module called deasync to force everything into a sync, which is a bit of a hack, but works for now. Hopefully there's a good resolution. I can totally understand not wanting to change getSwarm though.

@lorentzkim If you really need this feature, you could send in a PR. I'm willing to consider making getSwarm async, but we'll need to bump the major version since folks may be depending on that.
I don't think getSwarm is used in that many places, so this might not be too hard of a change.

I'll consider doing that.
Do you mind if async module gets added in as a dependency?

Don't mind the async thing, looks like you're using run-series and that does about the same for this purpose.

Yeah, run-series is equivalent to async.series without all the other cruft.

@lorentzkim any progress? I'm happy to take a look at an in-progress PR for this and share my feedback.

Please see my asnyc_getswarm branch. I was last changing out server.js:444.

Rudimentary changes should be done now. Take a look.

I am feeling a little iffy about the whole filters and the callback value; perhaps expand the result value to have more meaning than true/false? It already returns err for errors, if there are any. Custom filter reasons perhaps that gets passed to the server correctly?
Might also make sense to drop the infoHash+params thing to just params, since the function signatures are changing anyhow.

Released as 5.0.0.

Stunning work, thanks!

No problemo!


Fix edit labels modelbody css issue

@SanjalKatiyar , @GowthamShanmugam Please review

/test odf-console-e2e-aws

i feel label box is bigger, Please check with UX team. but as per design this is small like text input box

that is what I am saying... as per ur attached SS... "Edit labels" for DRPolicy looks diff from other places...
"Edit labels" at all the places in ODF/MCO should have original height... only for DRPolicy manage/apply page we need to decrease the height for the input box...

Sorry, I misunderstood that we need to reduce for edit label also. I will correct this.

LGTM, Please check once with latest master


[Server][Frontend] Implement JWT API token generation for programatic access

/api/stable/auth/token/generate endpoint with configurable permissions list, so the generated API token can be limitted, and configurable TTL.
Frontend:

Login using JWT
Generating JWT by specyfing list of roles + Time To Live (expiration time / TTL)
Revoking access to generated JWT

Done.


Update hooks-reference.md

Thanks for sending a PR. Seems like this was fixed. Sorry for the late turn around time.


Refactor

The app has been growing organically for a while and could use a decent refactor. Its hard to point to any specific place within the app where this should be done, but overall the code quality code be improved.

Also rename the function with the silly name populate_config_with_data_data_filepaths()


Object mapping issue and failed to execute bulk item

Hi
I am getting following issue shown in elasticsearch.log
[2019-08-19T16:03:30,213][DEBUG][o.e.a.b.TransportShardBulkAction] [9pMb_Kb] [cloudflare-2019-08-19][0] failed to execute bulk item (index) index {[<cloudflare-{2019-08-19||/w{yyyy-MM-dd|UTC}}>][doc][ezrCqWwBC8sWyWQexwgZ], source[n/a, actual length: [2kb], max length: 2kb]} org.elasticsearch.index.mapper.MapperParsingException: object mapping for [ClientIP] tried to parse field [ClientIP] as object, but found a concrete value 	at org.elasticsearch.index.mapper.DocumentParser.parseObjectOrNested(DocumentParser.java:360) ~[elasticsearch-6.7.2.jar:6.7.2]
Moreover I have opened the thread in elastic forum but didn't get any solution.
https://discuss.elastic.co/t/object-mapping-issue/195739/15
I would appreciate your consideration of this matter
Regards,
Usama

Please advise, How to get rid off this issue ?

@redhawk19 What version of Elasticsearch are you on?


Error

There is error zero-sized array to reduction operation maximum which has no identity
in download_historical_prices.py

Can I check that you've done the following:

Updated all your dependencies with pip install -r requirements.txt
Using python 3.6
Followed the instructions regarding historical data.

If you still get the same error, please paste it here and we'll try to figure out what's wrong :)
Robert

Closing for now, please let me know if it's still a problem


[uptime-kuma on Docker]Where is the certificate info?

⚠ Please verify that this bug has NOT been raised before.

[X] I checked and didn't find similar issue

🛡 Security Policy

[X] I agree to have read this project Security Policy

📝 Describe your problem
# first use
docker run -d --restart=always -p 3002:3001 -v uptime-kuma:/app/data --name uptime-kuma louislam/uptime-kuma:1
# second use
docker run -d --restart=always -p 3002:3001 -v uptime-kuma:/app/data --name uptime-kuma louislam/uptime-kuma:latest
# third use
docker run -d --restart=always -p 3002:3001 -v uptime-kuma:/app/data --name uptime-kuma louislam/uptime-kuma:beta

TLS Certificate Expiry not working

🐻 Uptime-Kuma Version
1.19.0-beta.2
💻 Operating System and Arch
Ubuntu 22.04.1 LTS x86_64
🌐 Browser
Google Chrome 108.0.5359.100
🐋 Docker Version
Docker version 20.10.21, build baeda1f
🟩 NodeJS Version
No response

Did you activated it on individual monitor?

Did you activated it on individual monitor?

of course.

Can't say, in my case, it appears on a 5th column.
Wait for someone who can help you.

Can't say, in my case, it appears on a 5th column. Wait for someone who can help you.
what is your uptime-kuma version? Do you run it on docker?

Yes, docker with Caddy as reverse proxy. v.1.18.5
I've tried your site.

1.18.5
Yes, docker with Caddy as reverse proxy. v.1.18.5 I've tried your site.

I tried this version, but still no have the cert exp tab.

Yes, docker with Caddy as reverse proxy. v.1.18.5 I've tried your site.

Can you remove this picture from the issue or put the mosaic on domain, thanks

2 possible situations that the grid is not shown:

Uptime Kuma fail to retrieve the cert details (Maybe you can some error log)
URL not start with https://

Yes! docker logs show the message like
2022-12-15T09:22:43.656Z [MONITOR] INFO: Get Monitor: 1 User ID: 1
2022-12-15T09:22:54.419Z [MANAGE] INFO: Resume Monitor: 1 User ID: 1
2022-12-15T09:22:54.426Z [MONITOR] INFO: Get Monitor: 1 User ID: 1
2022-12-15T09:22:54.773Z [MONITOR] WARN: Monitor #1 'tt.onevat.cn': Failing: Request failed with status code 400 | Interval: 20 seconds | Type: http | Down Count: 0 | Resend Interval: 0
2022-12-15T09:23:10.296Z [MANAGE] INFO: Resume Monitor: 1 User ID: 1
2022-12-15T09:23:10.303Z [MONITOR] INFO: Get Monitor: 1 User ID: 1
2022-12-15T09:23:10.458Z [MONITOR] ERROR: Caught error
2022-12-15T09:23:10.458Z [MONITOR] ERROR: res.request.res.socket.getPeerCertificate is not a function
2022-12-15T09:23:30.919Z [MONITOR] ERROR: Caught error
2022-12-15T09:23:30.919Z [MONITOR] ERROR: res.request.res.socket.getPeerCertificate is not a function
2022-12-15T09:23:51.123Z [MONITOR] ERROR: Caught error
2022-12-15T09:23:51.123Z [MONITOR] ERROR: res.request.res.socket.getPeerCertificate is not a function

Did you try other https like https://github.com

Did you try other https website like https://github.com

Did you try other https website like https://github.com

I ran the below commands, but still no have certificate info.
docker rm -f uptime-kuma && docker rmi louislam/uptime-kuma
docker volume rm uptime-kuma
mkdir /tmp/uptime-kuma
docker run -d --restart=always -p 3002:3001 -v /tmp/uptime-kuma:/app/data --name uptime-kuma louislam/uptime-kuma:beta

Did you try other https website like https://github.com

Run the application on non China mainland VPS is working.

It is a known issue, probably related to your server, but I can't tell what is the exact problem, because it is hard to reproduce.

It is a known issue, probably related to your server, but I can't tell what is the exact problem, because it is hard to reproduce.

Yes, Please delete this issue


TestCase for HHH-14944

Testcase for HHH-14944 Rebased to current state of 5.6 branch

Was fixed via https://github.com/hibernate/hibernate-orm/commit/5201a45055567c848821dd5eb37b7a81c259a22e


Add support to delete ec2 key pairs using cloud-nuke

Description
Fixes https://gruntwork.atlassian.net/browse/CORE-284.
Testing

Tested running the cloud-nuke command line locally to delete an existing EC2 key pair

aws-vault exec gruntwork-customer-access-test -- go run main.go aws --resource-type ec2-keypairs

Unit tests that creates & nuke an EC2 key pair.
Tested aws-inspect on ec2 key pair

aws-vault exec gruntwork-customer-access-test -- go run main.go inspect-aws --resource-type ec2-keypairs

TODOs
Read the Gruntwork contribution guidelines.

[x] Update the docs.
[x] Run the relevant tests successfully, including pre-commit checks.
[ ] Ensure any 3rd party code adheres with our license policy or delete this line if its not applicable.
[ ] Include release notes. If this PR is backward incompatible, include a migration guide.
[ ] Attention Grunts - if this PR adds support for a new resource, ensure the nuke_sandbox and nuke_phxdevops jobs in .circleci/config.yml have been updated with appropriate exclusions (either directly in the job or via the .circleci/nuke_config.yml file) to prevent nuking IAM roles, groups, resources, etc that are important for the test accounts.

Release Notes (draft)

Added / Removed / Updated [X].
Migration Guide

There's a folder in the repo called config that handles resources when using a config.yml file. The new resource should be added to the Config struct as well as any usages.
Readme also needs to be updated

Got it. Updated.

Need to update the readme and add release notes to the PR

Need to update the readme and add release notes to the PR

Hmm I though I updated the readme :S I guess it was not included somehow.
Don't we auto-generate the release note based on the PRs?
Is this one special because it's an open source repository?


versions: Update guest-components

The guest-components we build with the coco is far outdated with the KBS project. This commit updates the version of the guest-component we ship so that we can use latest KBS image.

Hey Suraj, do you think it is appropriate to get this PR merged with #1689 so that we pin guest-components and the KBS that they work with at the same time to avoid any drift between them?

Hey Suraj, do you think it is appropriate to get this PR merged with #1689 so that we pin guest-components and the KBS that they work with at the same time to avoid any drift between them?

@surajssd i made changes to mentioned PR and pointed out kbs image to lastest tag.

Hey Suraj, do you think it is appropriate to get this PR merged with #1689 so that we pin guest-components and the KBS that they work with at the same time to avoid any drift between them?

@surajssd i have made changes to my PR to include this change. Please check if that looks good to you. Thanks

Yep SGTM, closing this one.


Specifying self-defined output file name using something like --batch-file

Checklist

[x ] I'm reporting a site feature request
[x ] I've verified that I'm running youtube-dl version 2020.05.29
[x ] I've searched the bugtracker for similar site feature requests including closed ones

Description

WRITE DESCRIPTION HERE
Hi, can youtube-dl support using own specifying output file name template like --batch-file?
--batch-file requires a text file listing URLs
so can we customize output file name the same way?
Something like -o filename.txt
where filename.txt is like the text file used behind the command--batch-file but change all URLs into respective self-defined output file name,
For example, let's say I want to download a youtube videos and I wish the filename is 'xx.mp4',
so I write one text file pasting its URL saving as batchfile.txt,
then I write another text file pasting xx.mp4, which is the file name I want youtube-dl to use,
after that, saving it as filename.txt
Finally, using youtube-dl -o filename.txt --batch-file batchfile.txt resulting my desired outcome..

Not possible. By design -o specifies an output template not a file with output templates.


Problems encountered when running the app

Installing and getting the app up was pretty simple and straightforward.
The problems started when trying to feed it data.
I cloned the app repo and checked out the Jamaica branch and copied the data fixtures used for testing. test/fixtures/data to web/
Since I noticed that there wasn't any .osm or .osrm file I checked the master branch. There wasn't any .osm file there as well but I found JM.pbg which I converted to .osm using this script.
I then followed the instruction on the OSRM import README.
Running the command ./osrm-extract map.osm gives this error:
[error] Profile profiles/car.lua not found!

so I tried ./osrm-extract -p profile.lua map.osm but I get:
[info] Using script profile.lua
[info] Input file: map.osm
[info] Profile: profile.lua
[info] Threads: 8
[info] Parsing in progress..
[STXXL-MSG] STXXL v1.4.1 (prerelease/Debug)
[STXXL-MSG] Disk '/var/tmp/stxxl' is allocated, space: 2500 MiB, I/O implementation: memory queue=0 devid=0
[info] input file generated by Osmosis 0.45
[info] timestamp: n/a
[info] Found no turn restriction tags
TBB Warning: Exact exception propagation is requested by application but the linked library is built without support for it
terminate called after throwing an instance of 'tbb::captured_exception'
  what():  lua: error: lua: error: profile.lua:388: sol: attempt to index (set) lua_nil value "is_access_restricted" on userdata (bad (misspelled?) key name or does not exist)
Aborted (core dumped)

I tried commenting the lines 381-389 in profile.lua and the processing worked. However I don't know the implications of doing this. It may remove important data.
I then tried to run the app with the new generated file, but I'm getting another error when I try to generate the dataset.
Location of OSRM:  /srv/rra/./web/data/project_1463767649122/baseline/JM.osrm
/srv/rra/scripts/node/calculateETA.js:26
  var osrm = new OSRM(e.osrm);
             ^

TypeError: Invalid file paths given!
    at TypeError (native)
    at process.<anonymous> (/srv/rra/scripts/node/calculateETA.js:26:13)
    at emitTwo (events.js:106:13)
    at process.emit (events.js:191:7)
    at processEmit [as emit] (/srv/rra/node_modules/signal-exit/index.js:155:32)
    at process.nextTick (internal/child_process.js:744:12)
    at _combinedTickCallback (internal/process/next_tick.js:67:7)
    at process._tickCallback (internal/process/next_tick.js:98:9)

I verified that the file does exist in that path, so I'm not sure why it is failing.
cc @olafveerman

Which OSRM version are you using? The profile.lua files and node-osrm are very sensitive to the specific OSRM versions. I generally build a release version of OSRM, use the profile.lua as it is included in the release tar ball and make sure that the node-osrm is using the same version.
Also OSRM can handle .pbf files as well, no need to convert to .osm

I'm trying to recreate it and I noticed that if you only do osrm-extract JM.pbf and not the next step osrm-contract JM.osrm you get the TypeError: Invalid file paths given! error.

@stvno
Ok, I tried to start from scratch. This is what I did this time:

Latest code from Jamaica branch
Copy test/fixtures/data to web
Get JM.pbf from master
Build osrm 5.4.0
Build failing. Solved with https://github.com/Project-OSRM/osrm-backend/issues/1196#issuecomment-277220177
Convert JM.pbf to JM.osrm, following the instructions in scripts/OSRM import
Copy JM.osrm* to web/data/project_1463767649122/baseline
Run

It works now. Clicking on a province generates the result csv, but it's empty.
The only output in the console is: No villages within batch.
Is the village data for another area?

@stvno figured out what was causing the problem.

In calculateETA.js#L189 villages is already a featureCollection
In calculateETA.js#L202 it is also already a featureCollection

Once you remove those functions it works. Not sure what impact this has but works for the demo data.

hm, I know the application assumes and doesn't check if data is already in a featurecollection. But not sure why it failed on the test data. @brunosan any idea why this fails?

With the new codebase, this is outdated.


Programatically set tint_background_colors

I'm trying to configure tint.nvim such that my NeoTree window doesn't get it's bg tinted, however I want everything else to be tinted.
I see that there is the tint_background_colors option, but it seems it's only possible to set this as a global option.
Ideally I would be able pass a function where I can inspect the window type and conditionally disable bg tinting for that window.
I initially looked into handling this with a custom transform, but when I saw this option it made me think that being able to pass a callback function to toggle this value per-window would be cleaner.

Thanks, although maybe I wasn't clear.
It seems that even when tint_background_colors is false and when the NeoTree window is not active, NeoTree appears to use the NormalNC.bg highlight. This doesn't seem to be expected?
If I disable tint.nvim, when NeoTree is not active it (correctly/expectedly) uses the NeoTreeNormalNC.bg highlight.
I can record a video demonstrating this - although first could you confirm whether it's expected that tint.nvim with tint_background_colors = false causes an in-active window to use NormalNC.bg for it's background?
Thanks!

I'm not 100% sure I understand the question correctly, but I would imagine that NeoTreeNormalNC is linked to NormalNC?


Black screen on launch

When I launch berry (no dm), it launches into a black screen, without a cursor.

Hello,
Same black screen after testing it on a VM with Ubuntu 18.04.
Do you have tips to help ? Logs somewhere ?
Thanks.

Do a chmod +x on the autostart file.

Unfortunately that didn't help

Try redirecting the output to a log file and post the output here. My guess is that berry isn't starting correctly but this should help narrow it down. Start berry with the command berry > /tmp/log.txt 2>&1 and upload the output here and I'll take a look.

Hi, thanks for your answer.
Here is the output of the berry command :
berry: font not specified, loading default font berry: Successfully opened display Fatal request. request code=2, error code=10 X Error of failed request:  BadAccess (attempt to access private resource denied)   Major opcode of failed request:  2 (X_ChangeWindowAttributes)   Serial number of failed request:  22   Current serial number in output stream:  24
I don't know why it attempt to access private resource though.
I got this error by running berry directly in my terminal, so X server was already started and used.

@Djamy berry cannot be started inside of another xsession. Does this error persist if you start berry by running startx inside of tty or by starting from a display manager.

Hi!
Same problem here, after installing berry and launch through a multiple .xinitrc, tryied only berry in the .xinitrc, but it didn't work black screen, no color background (I put a color background on autostart) and didn't work.
But if I launch Google Chrome is launched. But if I launch the terminal (xfce4-terminal) berry crashes and I go back to tty.
The autostart file is executable.
Any help?

Okay, so I believe that this is a problem with fonts. In fact, I tried to log the output as described here:

Try redirecting the output to a log file and post the output here. My guess is that berry isn't starting correctly but this should help narrow it down. Start berry with the command berry > /tmp/log.txt 2>&1 and upload the output here and I'll take a look.

and it threw a segfault when drawing text. I think this is because on voidlinux(what I'm currently using) no font is available when installing the xorg package. So I tried installing font-firacode and started the X session and it worked. Hope this helps somebody.

Okay, so I believe that this is a problem with fonts. In fact, I tried to log the output as described here:

Try redirecting the output to a log file and post the output here. My guess is that berry isn't starting correctly but this should help narrow it down. Start berry with the command berry > /tmp/log.txt 2>&1 and upload the output here and I'll take a look.

and it threw a segfault when drawing text (Same behavior as #229. I think this is because on voidlinux(what I'm currently using) no font is available when installing the xorg package. So I tried installing font-firacode and started the X session and it worked. hope it helps.

Um! I have xfonts installed and the blank screen remains... I can launch program but nothing about the background color, all black....

Aw yeah tough the topic was different. Have you tried hsetroot or feh to set the background

Yeah is set with hsetroot... But nothing! :cry:

@jolupa can you post your .xinitrc, your autostart, and a log file from crashing

Hi! sorry for the delay.
Here are my .xinitrc and autostart
Thanks.
.xinitrc
#!/bin/sh

#Xfce Default
session=${1:-xfce}

case $session in
	berry			)
		exec berry;;
	xfce			)
		exec ck-launch-session startxfce4;;
	*					)
		exec $1;;
esac

Autostart
#!/bin/bash

source $HOME/.bashrc
sxhkd -c $HOME/.config/berry/sxhkdrc &
$HOME/.fehbg

setxkbmap -layout us -variant altgr-intl

# start bars
#tint2 -c $HOME/dots/tint2/time &
#tint2 -c $HOME/dots/tint2/desktop &

berryc top_gap 0

berryc border_width 0
berryc inner_border_width 5
berryc title_height 20

berryc focus_color 292d3e
berryc unfocus_color 292d3e
berryc inner_focus_color b89ca8
berryc inner_unfocus_color 343d41
berryc text_focus_color 2c3539
berryc text_unfocus_color 565679

@jolupa can you post a log file from the crash? Simply start berry with:
berry > /tmp/log.txt 2>&1

@JLErvin Here's the log file, not any error except for the font.
I think that I made something wrong. Now I have a background, the one I want, but the mouse pointer is not showing 'till the first window is open.
Don't know... :stuck_out_tongue:
log.txt

@jolupa this is the whole log file?! That isn't even an error - that's just berry letting us know that you didn't specify a font via the command line and it will instead load the default font (i.e. Monospace 10) from config.h. If there's more to the log please post it.

@JLErvin no that's all. Viewing the log I think that the problem, maybe, is in my side. As I told you beofore, now I have background and the only weird thing is that I don't have cursor till the first window is open.

@jolupa oh, this is the correct behavior.

@JLErvin Hello, I'm still struggling with this and I cannot start berry from gdm3..
I traced the syslog and found a segfault from libc-2.27.so (and a fantastic "fuck me" in it :rofl: )
Could you help to understand what is wrong and why I can't use your wm, please ?
berry_syslog.log

@Djamy I recently added a fix that was causing crashes on the development branch. Can you try cloning and remaking the project from here and see if it fixes the issue.
P.S. I need to screen my commits better next time haha

@JLErvin Thank you, it seems to solve my problem.
Now I'm facing a kind of blank screen but it is supposed to be the correct behavior as you said, still how can I have the mouse cursor or even launch programs ?

@Djamy enough people were requesting a default cursor that I just added one as of 0.1.2
With regards to launching applications this is something you will have to do on your end. Following the traditional unix philosophy, berry doesn't handle something like launching applications for you. If you are looking for good application launchers I personally use dmenu although I know many people that also use rofi.
To set a background I recommend a program like hsetroot

I'm experiencing the same problem on Ubuntu 19.04. Starting from gdm just loops me back there. Starting from CLI I receive:
berry: font not specified, loading default font
That's it. It seems like this has been solved for others above, but I'm still having trouble.

@JLErvin Okay thank you, I'll see how I can handle this on myself.
@vaskark You should try to checkout the blackberry branch :

@Djamy I recently added a fix that was causing crashes on the development branch. Can you try cloning and remaking the project from here and see if it fixes the issue.
P.S. I need to screen my commits better next time haha

@Djamy Ok. I'm on dev branch. Loads fine now, except for font.

@vaskark what happens with the font? If you are getting that message but the fonts are still drawn then there is nothing wrong - it must means you aren't specifying a font at startup with the -f flag.

@JLErvin My apologies. I didn't have it formated properly in my autostart above. All is fine now :)
It's a neat wm. Congrats, and thank you.
P.S. I'm starting berry from gdm. Is there an exit command I can set in sxhkdrc?

@vaskark This has also been a requested feature so I'll add a dedicated command in a future update. Until then that works fine, however.

@vaskark berryc quit has been implemented as of 0.1.3. Closing this issue due to inactivity.


forc run - allow for specifying main arguments for scripts

Currently, using forc run there is no way to provide main arguments for scripts.
CLI arguments
We couild at least allow for specifying primitive arguments via the command line, e.g.
forc run -- arg1 arg2 arg3

script-args.json or similar?
We could also settle on a file format (toml or json) that provides both the argument values and their types so that the user can update their file and do something like forc run --args script-args.json or similar.
We might need to support some sort of args file like this either way if we want to allow users to submit more sophisticated types via main arguments. Perhaps we can extract the fuels representation into fuels-types (if it isn't already) so that we can re-use the same serializable main args format directly.
This is not a big priority for users already using the Rust or TS SDKs directly to run scripts, but a feature like this would be especially useful for users with simpler scripts, or whose Sway projects are embedded in other languages.
cc @nfurfaro @simonr0204 let us know if you have any more thoughts / ideas on this!

Going to look into picking this up. Are there any updates/changes in the requirements I should know of? cc @nfurfaro @simonr0204

@bingcicle I would be interested in picking this up if you wanted to focus more on the package registry stuff? No worries if you want to keep going with it though.

@nfurfaro Yes, for sure! I'll unassign myself and assign you instead :+1:

Started looking into this, seems like we already depend on the SDK and SDK handles this pretty nice, so maybe we should remove the tx creation implementation in forc run and make forc run a cli wrapper around the SDK's script functionality. Any thoughts?

Also to add more context, this was not possible to be done in a safe way since we need a WalletUnlocked instance to be created by the user's input in the CLI. But since we have forc-wallet as a lib now, we can get the WalletUnlocked safely from the user's vault, and sign operation can be done automatically. This enable us to use SDK's already established workflow.

Build the project obtain the artifacts.
Create a provider using node_url passed by the user.
If user specified --unsigned, create a dummy/default wallet. Otherwise call forc-wallet as a lib, and obtain a WalletUnlocked safely.
With the obtained WalletUnlocked (which is created by using the provider and forc-wallet invocation/default wallet) call the main function of the script like in the example of SDK