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.

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

Search sessions or speakers by voice-search #776

Merged
merged 9 commits into from
Feb 16, 2020

Conversation

imsakuu
Copy link
Member

@imsakuu imsakuu commented Feb 13, 2020

@takahirom
Copy link
Member

Thanks!
I merged this PR
Can you check after merge master? 🙇
#781

@imsakuu
Copy link
Member Author

imsakuu commented Feb 15, 2020

@takahirom
Sure. I'll do it!
btw, which should I use "merge" or "rebase" just in case?
Sorry to a basic question.🙇‍♂️

@takahirom
Copy link
Member

I think both rebase and merge have good points, so I'm not particular about the policy. Please do it any way you like. 😄

@takahirom
Copy link
Member

Sorry please wait this #787

@takahirom
Copy link
Member

I merged it 🙇

@imsakuu
Copy link
Member Author

imsakuu commented Feb 15, 2020

I think both rebase and merge have good points, so I'm not particular about the policy. Please do it any way you like. 😄

That's right. Thanks for answer.😳

@@ -10,11 +10,15 @@
android:supportsRtl="false"
android:usesCleartextTraffic="true"
Copy link
Collaborator

@jmatsu-bot jmatsu-bot Feb 15, 2020

Choose a reason for hiding this comment

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

⚠️ Attribute usesCleartextTraffic is only used in API level 23 and higher (current min is 21)

@imsakuu
Copy link
Member Author

imsakuu commented Feb 15, 2020

I think no problem between #781 and mine.
However, I have no confidence about my code💦

@takahirom takahirom added this to the 1.1.0 milestone Feb 15, 2020
@@ -148,10 +153,27 @@ class SearchSessionsFragment : Fragment(R.layout.fragment_search_sessions), Inje
}
}

override fun onResume() {
super.onResume()
val intent = activity!!.intent
Copy link
Member

Choose a reason for hiding this comment

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

You can use requireActivity() 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed it!😊

val intent = activity!!.intent
if (intent.action == Intent.ACTION_SEARCH) {
val query: String = intent.getStringExtra(SearchManager.QUERY)!!
val searchResult = searchSessionsViewModel.uiModel.requireValue().searchResult
Copy link
Member

Choose a reason for hiding this comment

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

I think we don't need to check to change query 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought so.
However, onResume is called after setQuery then it's query is same.
I want to only use once setQuery but I have no idea w/o query != searchResult.query. 😂

Copy link
Member

Choose a reason for hiding this comment

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

It's not so many times, so I think it's okay to call twice, but is there any problem? 👀

Copy link
Member Author

@imsakuu imsakuu Feb 16, 2020

Choose a reason for hiding this comment

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

Not twice. It loops onResume -> setQuery -> onResume.
For example, below code.

    override fun onResume() {
        super.onResume()
        val intent = requireActivity().intent
        if (intent.action == Intent.ACTION_SEARCH) {
            val query: String = requireNotNull(intent.getStringExtra(SearchManager.QUERY))
            val searchView: SearchView? =
                menu?.findItem(R.id.search_view)?.actionView as SearchView?
            if (searchView != null) {
                searchView.setQuery(query, true)
            }
        }
    }

imsakuu and others added 2 commits February 16, 2020 10:04
…20/session/ui/SearchSessionsFragment.kt

Co-Authored-By: Takahiro Menju <takam.dev@gmail.com>
@takahirom
Copy link
Member

When we use light theme. we can not see voice icon 😭

@imsakuu
Copy link
Member Author

imsakuu commented Feb 16, 2020

Yes. i knew it 😂
I tried to apply voice icon design.
But I stopped it and deleted codes when I noticed #437 ...💦

@imsakuu
Copy link
Member Author

imsakuu commented Feb 16, 2020

I can try again about voice icon if you can wait🙇‍♂️

@takahirom takahirom removed this from the 1.1.0 milestone Feb 16, 2020
@takahirom
Copy link
Member

I can try again about voice icon if you can wait🙇‍♂

Yes! You can this in 1.2 or later 👍

@takahirom
Copy link
Member

I pushed a little refactoring!

…20/session/ui/SearchSessionsFragment.kt

Co-Authored-By: Takahiro Menju <takam.dev@gmail.com>
@imsakuu
Copy link
Member Author

imsakuu commented Feb 16, 2020

I pushed a little refactoring!

I have many things to learn from this refactoring 👀
Thanks!😊

@imsakuu
Copy link
Member Author

imsakuu commented Feb 16, 2020

I pushed about voice icon changes!
But icon color is different from the design 😢 figma

Light Dark

@takahirom
Copy link
Member

Can you check this?
./gradlew :feature:session:ktlint

@jmatsu-bot
Copy link
Collaborator

Your apk has been deployed to https://deploygate.com/distributions/76f0306b29d4cd5fb9c4c4c6afee4f48bc27b78e. Anyone can try your changes via the link.

Generated by 🚫 Danger

@jmatsu-bot
Copy link
Collaborator

1 Warning
⚠️ android-base/src/main/AndroidManifest.xml#L5 - On SDK version 23 and up, your app data will be automatically backed up, and restored on app install. Your GCM regid will not work across restores, so you must ensure that it is excluded from the back-up set. Use the attribute android:fullBackupContent to specify an @xml resource which configures which files to backup. More info: https://developer.android.com/training/backup/autosyncapi.html

No error was reported but at least one warning was found.

Generated by 🚫 Danger

@imsakuu
Copy link
Member Author

imsakuu commented Feb 16, 2020

Can you check this?
./gradlew :feature:session:ktlint

I fixed 🙏

@takahirom
Copy link
Member

Thanks! LGTM 👍

@takahirom takahirom merged commit 8c1b6d7 into DroidKaigi:master Feb 16, 2020
@imsakuu
Copy link
Member Author

imsakuu commented Feb 16, 2020

Thanks for so kind reviews 😭
It helps me a lot 🙇‍♂️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Search sessions or speakers by your voice in search screen
3 participants