Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

secondlife/viewer#1330 EmojiPicker inserts emoji to inactive chat floater #1348

Merged
merged 1 commit into from May 2, 2024
Merged

secondlife/viewer#1330 EmojiPicker inserts emoji to inactive chat floater #1348

merged 1 commit into from May 2, 2024

Conversation

LLGuru
Copy link
Contributor

@LLGuru LLGuru commented Apr 28, 2024 •
edited

Fix for #1330

Use-case A: EmojiPicker should be hidden when the modal (toast) floater is shown - indra\newview\llscreenchannel.cpp
Use-case B: EmojiPicker should be hidden when the top menu is shown - indra\llui\llmenugl.cpp
Use-case C: All menus should be hidden when the modal (toast) floater is shown - indra\llui\llmodaldialog.cpp

@LLGuru LLGuru linked an issue Apr 28, 2024 that may be closed by this pull request
EmojiPicker inserts emoji to inactive chat floater when other modal floater is focused #1330
Open
@LLGuru LLGuru mentioned this pull request Apr 28, 2024
Open
akleshchev
akleshchev reviewed Apr 29, 2024
View reviewed changes
indra/llui/llfloater.cpp Outdated
@@ -659,6 +659,8 @@ void LLFloater::openFloater(const LLSD& key)

LLViewerEventRecorder::instance().logVisibilityChange( getPathname(), getName(), true,"floater"); // Last param is event subtype or empty string

LLMenuGL::sMenuContainer->hideMenus();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scenario:
Open a 'Build->uploads menu'
Detach menu (two lines at the top of 'uploads' sub menu)
Expected: for main menu not to close and allow user to detach more menus

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open a 'Build->uploads menu'

Which menu item do you mean?
image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Detach menu (two lines at the top of 'uploads' sub menu)

Screenshot please

akleshchev
akleshchev requested changes Apr 29, 2024
View reviewed changes
Copy link
Contributor

@akleshchev akleshchev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If any floater popup is going to close menu, it will be imposible to navigate menus while receiving script questions, IMs and chat messages (with closed chat they pop up as toasts).

Toasts are supposed to be non distruptive to your current activities including menu navigation.

@LLGuru
Copy link
Contributor Author

LLGuru commented Apr 29, 2024

Toasts are supposed to be non distruptive to your current activities including menu navigation.

When you click the toast area outside its buttons, the opened top menu becomes hidden
I believe this is the state which should start immediately after the toast is shown, without any click on it

@LLGuru
Copy link
Contributor Author

LLGuru commented Apr 29, 2024

If any floater popup is going to close menu, it will be imposible to navigate menus while receiving script questions

If the floater doesn't close the menu, it has a chance to be shown behind this menu - is this OK, you mean?

@LLGuru LLGuru force-pushed the guru/viewer-1330-emojipicker branch from af044eb to fdfae4a Compare April 29, 2024 13:05
@akleshchev
Copy link
Contributor

If the floater doesn't close the menu, it has a chance to be shown behind this menu - is this OK, you mean?

Of course. Script questions, IMs and the like aren't supposed to interrupt your menu navigation, they are supposed to be in background untill you decide it's time check it.

@LLGuru LLGuru requested a review from akleshchev April 29, 2024 13:08
@LLGuru
Copy link
Contributor Author

LLGuru commented Apr 29, 2024 •
edited

IMs and the like aren't supposed to interrupt your menu navigation

Moved hideMenus() into LLModalDialog::onOpen inside if (mModal)
Now, only modal floaters will hide the menu, but IM toasts etc. won't

@LLGuru LLGuru marked this pull request as ready for review April 29, 2024 13:15
@LLGuru LLGuru force-pushed the guru/viewer-1330-emojipicker branch from fdfae4a to d1da7fe Compare April 29, 2024 13:25
akleshchev
akleshchev requested changes Apr 29, 2024
View reviewed changes
indra/newview/llscreenchannel.cpp Outdated Show resolved Hide resolved
@nat-goodspeed nat-goodspeed removed their request for review April 29, 2024 21:18
@nat-goodspeed
Copy link
Collaborator

I don't have a good intuition for behavior in this area, sorry.

@LLGuru LLGuru force-pushed the guru/viewer-1330-emojipicker branch from d1da7fe to dca08c0 Compare April 29, 2024 22:24
@LLGuru LLGuru requested a review from akleshchev April 29, 2024 22:24
@LLGuru
Copy link
Contributor Author

LLGuru commented Apr 29, 2024

I don't have a good intuition for behavior in this area, sorry.

Your participation was suggested by github, sorry.

@LLGuru LLGuru force-pushed the guru/viewer-1330-emojipicker branch 2 times, most recently from 649dd8c to 56cf54f Compare May 1, 2024 18:20
akleshchev
akleshchev approved these changes May 1, 2024
View reviewed changes
indra/newview/llscreenchannel.cpp Outdated
if (toast->isModal())
{
// If Modal, hide other UI areas temporally shown
toast->hideOtherViews();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personaly I would have prefer for this to be in LLModalDialog::setVisible
There is very little reason for this code to be here, in LLScreenChannel::addToast

indra/llui/llmodaldialog.cpp Outdated
@@ -345,3 +348,13 @@ void LLModalDialog::shutdownModals()
// simply makes our software look unreliable.
sModalStack.clear();
}

// static
void LLModalDialog::hideOtherViews()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Name is very misleading, while meanus are certainly a view, everything else is too.

@LLGuru LLGuru force-pushed the guru/viewer-1330-emojipicker branch from 56cf54f to e4e0bdd Compare May 1, 2024 22:19
@LLGuru LLGuru force-pushed the guru/viewer-1330-emojipicker branch from e4e0bdd to bf26be1 Compare May 2, 2024 00:15
@LLGuru LLGuru merged commit faefd35 into release/maint-c May 2, 2024
10 checks passed
@LLGuru LLGuru deleted the guru/viewer-1330-emojipicker branch May 2, 2024 01:39
@github-actions github-actions bot locked and limited conversation to collaborators May 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@akleshchev akleshchev akleshchev approved these changes

@marchcat marchcat marchcat approved these changes

Assignees
No one assigned
Labels
c/cpp llui
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

EmojiPicker inserts emoji to inactive chat floater when other modal floater is focused
4 participants
@LLGuru @akleshchev @nat-goodspeed @marchcat