Sternbach-Software / GeniForAndroid

Geni.com Android app
GNU General Public License v3.0
3 stars 1 forks source link

Fix a few magic nums to test proj activity #16

Open whittede opened 1 year ago

whittede commented 1 year ago

Testing the waters by fixing a few magic numbers per issue https://github.com/Sternbach-Software/GeniForAndroid/issues/9, which I think I did right. Using this pull request to see if the project is still active.

whittede commented 1 year ago

Hey, just for a learning experience here so I can improve for next time:

These consts should be part of an intdef.

These constants were implemented as a part of an IntDef in app/src/main/java/app/familygem/constant/intdefs/FabMenuOptions.kt. They were added to lines 9 - 16. However, is the issue that they should have gone in their own IntDef Kotlin file instead of FabMenuOptions.kt? If that's the issue, then I can make that change.

Unless I'm somehow understanding IntDefs completely wrong in my usage of them.. But this seemed to be how the other IntDefs in the project were implemented.

Besides, I believe I already implemented this pushed.

Oops, if so that's my bad. I pulled from the most recent, up-to-date copy of the master branch that was available this month. In the future, where can I pull your pushed changes from to avoid duplicating work if they are not in the master branch?

whittede commented 1 year ago

@Sternbach-Software pinging about the above response I left ⬆ just so I can learn for next time. 👍 Thanks!

Sternbach-Software commented 1 year ago

In this diff, I don't see a decleration of those constants referenced in an @IntDef. Example from docs:

@Retention(SOURCE)
@IntDef({NAVIGATION_MODE_STANDARD, NAVIGATION_MODE_LIST, NAVIGATION_MODE_TABS})
public @interface NavigationMode {}
public static final int NAVIGATION_MODE_STANDARD = 0;
public static final int NAVIGATION_MODE_LIST = 1;
public static final int NAVIGATION_MODE_TABS = 2;
...
public abstract void setNavigationMode(@NavigationMode int mode);

@NavigationMode
public abstract int getNavigationMode();
whittede commented 1 year ago

In this diff, I don't see a decleration of those constants referenced in an @IntDef.

In the diff you linked there, I see NOT_EDIT_MODE, EDIT_MODE, MAIN_AUTHOR_OPTION, ALL_OTHER_OPTIONS, replacing magic numbers in .../DetailActivity.kt and I see them all being inserted as @IntDefs into the .../FabMenuOptions.kt file in the same way that the example from the docs are using them.

@Retention(AnnotationRetention.SOURCE)
annotation class FabMenuOptions
const val ADDRESS_OPTION = 0
const val DELETE_OPTION = 0
const val NOT_EDIT_MODE = 0
const val EDIT_MODE = 1
const val MAIN_AUTHOR_OPTION = 1
const val CROP_OPTION = 2
const val CHOOSE_OPTION = 3
const val FAMILY_OPTION = 4
const val ALL_OTHER_OPTIONS = 5

In fact, there were already @IntDef constants declared in that file (.../FabMenuOptions.kt) and used in the project just fine, seemingly.

I'm sorry that I'm being so dense in understanding this, but could you explain what about the new constants added weren't correctly declared in an @IntDef? (And also, as I said up here, if the issue is just that the @IntDef needs to declare those constants in a separate file instead of in .../FabMenuOptions.kt then just let me know.)

Sternbach-Software commented 1 year ago

Ah, yes - you are correct! So sorry I missed that! I really do apologize for driving you crazy - I simply didn't see the @IntDef!