IDE-51 Extend "Search" feature to names of scenes, groups, objects, looks, and sounds#5057
IDE-51 Extend "Search" feature to names of scenes, groups, objects, looks, and sounds#5057p0dlunsek wants to merge 1 commit intoCatrobat:developfrom
Conversation
|
|
Please keep as draft until finished |
p0dlunsek
changed the title
|
p0dlunsek
force-pushed
the
IDE-51
branch
2 times, most recently
from
df25d07 to
81fe8b7
Compare
|
reichli
added
the
Active Member
reichli
left a comment
reichli
left a comment
There was a problem hiding this comment.
Hi!
I tested the functionality and it works.
Most comments I have are about code structure and simplification:
- I left some suggestions on how the code could be simplified as review comments. Please have a look at them and let me know if they do not work with your current solution or are impractical for any other reason.
- Please rebase the branch ontop of the most recent develop-branch.
- I also some some inconsistent formatting across the files (newlines, spaces, structuring of if/else-blocks). Please use the Android Studio Formatter on all files that you touched in this PR. many of those formatting inconsistencies also show up during linting (Checkstyle, SonarCube).
| if(isThisFoundObject(position)){ | ||
| highlightTheFoundObject(holder); | ||
| } | ||
| else{ |
There was a problem hiding this comment.
Please run the Android Studio Formatter on changes like this (correct formatting of if/else-blocks and brackets) and others in all touched files of this PR.
| * along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
| */ | ||
|
|
||
| package org.catrobat.catroid.test.ui |
There was a problem hiding this comment.
Please move this test to the espresso-package
| FinderDataManager.instance.currentMatchIndex = brickIndex | ||
|
|
||
| when (type) { | ||
| 1 -> activity.onBackPressed() |
There was a problem hiding this comment.
Please use the enum values (FinderDataManager.FragmentType.SCENE) instead of integers. This makes the code way more comprehensible. This appears multiple times in the PR, not just in this file.
|
|
||
| private var initiatingfragment:FragmentType = FragmentType.NONE | ||
| private var initiatingPosition = arrayOf(-1,-1,-1) | ||
| private var searchResults = mutableListOf<Array<Int>>() |
There was a problem hiding this comment.
Have you thought about creating a simple java class (like a c++ struct) for the search results instead of using integer arrays? This would make the code more readable in many places and potentially simpler.
We could also simplify the interface OnResultFoundListener.onResultFound, use equality-checks to check whether a search result already exists in Finder, etc.
| result.let { | ||
| FinderDataManager.instance.getSearchResults().size.let { it1 -> | ||
| onResultFoundListener?.onResultFound( | ||
| it[0], it[1], it[2],it[3], it1, |
There was a problem hiding this comment.
This section's readability would greatly profit from using an java object to represent a search result instead of integers arrays.
| private val searchResultsNames = mutableListOf<String>() | ||
| private var searchResultIndex = -1 | ||
| private var searchQuery: String? = null | ||
| private var searchOrder = arrayOf(1,2,3,4,5) |
There was a problem hiding this comment.
Is there a reason for this variable? Currently the search order in Finder is constant
|
|
||
| interface OnResultFoundListener { | ||
| fun onResultFound( | ||
| sceneIndex: Int, |
There was a problem hiding this comment.
Some suggestions that might make sense to simplify the code:
totalResultsapparently is never used -> please remove- I think the name
brickIndexshould be changed to something else, since bricks aren't the only entity that can be found (maybeelementIndex?). - change
typefromInttoFragmentType? - If we have a own class for the search result, all these parameters can be collapsed into one.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Checkstyle found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
There was a problem hiding this comment.
detekt found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
| } | ||
|
|
||
| val isClosed: Boolean | ||
| get() = visibility == GONE |
Check warning
Code scanning / Android Lint
Use KTX extension function Warning
| case R.id.merge: | ||
| startActionMode(MERGE); | ||
| break; | ||
| case R.id.find: |
Check warning
Code scanning / Android Lint
Checks use of resource IDs in places requiring constants Warning
| } else { | ||
| textView?.text = createActionBarTitle() | ||
| initializeAdapter() | ||
| adapter.notifyDataSetChanged() |
Check warning
Code scanning / Android Lint
Invalidating All RecyclerView Data Warning
p0dlunsek
force-pushed
the
IDE-51
branch
2 times, most recently
from
fcd2763 to
be1b068
Compare
|
4 participants
https://catrobat.atlassian.net/jira/software/c/projects/IDE/boards/124?selectedIssue=IDE-51