[FEATURE REQUEST] Create a link over a space #4794
Conversation
joragua
force-pushed
the
feature/create_a_link_over_a_space
branch
from
1d538ec to
4c0e841
Compare
joragua
force-pushed
the
feature/create_a_link_over_a_space
branch
4 times, most recently
from
e746700 to
dab471c
Compare
joragua
force-pushed
the
feature/create_a_link_over_a_space
branch
6 times, most recently
from
d6efcfa to
3f3ec03
Compare
joragua
force-pushed
the
feature/create_a_link_over_a_space
branch
2 times, most recently
from
cefab52 to
73259d0
Compare
joragua
force-pushed
the
feature/create_a_link_over_a_space
branch
from
73259d0 to
e3a9230
Compare
| setPasswordSwitch.isVisible = !isPasswordEnforced | ||
| setPasswordSwitch.isChecked = hasPassword | ||
| } | ||
| binding.createPublicLinkButton.isEnabled = isPasswordEnforced && hasPassword || !isPasswordEnforced |
There was a problem hiding this comment.
What would happen if !isPasswordEnforced and permission is not selected yet? OTOH, shouldn't parentheses be necessary?. Not the same:
binding.createPublicLinkButton.isEnabled = (isPasswordEnforced && hasPassword) || !isPasswordEnforced
binding.createPublicLinkButton.isEnabled = isPasswordEnforced && (hasPassword || !isPasswordEnforced)
i guess the first one is the correct, the one you wanted... and probably the one that kotlin will take. Just in case.
There was a problem hiding this comment.
That code is wrapped in selectedPermission?.let{ } block, so the logic doesn't apply when the permission is not selected. Anyway, I'll add parentheses in the condition to make it clearer. The final version:
binding.createPublicLinkButton.isEnabled = (isPasswordEnforced && hasPassword) || !isPasswordEnforced
| } | ||
| binding.createPublicLinkButton.isEnabled = isPasswordEnforced && hasPassword || !isPasswordEnforced | ||
|
|
||
| binding.createPublicLinkButton.setOnClickListener { |
There was a problem hiding this comment.
Question: since the "set listeners" (this and the following ones) are inside of the collectLatestLifecycleFlow , are they registered in every state change?
| } | ||
|
|
||
| private fun checkPasswordEnforced(selectedPermission: OCLinkType) { | ||
| isPasswordEnforced = when (selectedPermission) { |
There was a problem hiding this comment.
this kind of decision, aren't a ViewModel responsibility, instead of the fragment?
|
|
||
| private val spaceLinksViewModel: SpaceLinksViewModel by activityViewModel { | ||
| parametersOf( | ||
| requireArguments().getString(ARG_ACCOUNT_NAME), |
There was a problem hiding this comment.
I noticed requireArguments().getString(ARG_ACCOUNT_NAME) to be called several times through the class. Couldn't be refactored to be read only once or similar?
| canEditPublicLinkRadioButton.isChecked = false | ||
| secretFileDropPublicLinkRadioButton.isChecked = false | ||
| selectedRadioButton.isChecked = true | ||
| } |
There was a problem hiding this comment.
About this block: you set all the permissions to false and then, the selected one to true. Why that? is it required to reset all the buttons before setting a new value?
EDIT: after checking the correspondent layout, i noticed you use three individual RadioButton . Would it be better to use a RadioGroup that will make all options exclusive? https://developer.android.com/reference/android/widget/RadioGroup
| android:id="@+id/options_title" | ||
| android:layout_width="wrap_content" | ||
| android:layout_height="50dp" | ||
| android:layout_centerVertical="true" |
There was a problem hiding this comment.
i think this is not useful in a ConstraintLayout
| android:background="@android:color/darker_gray" | ||
| android:alpha="0.5"/> | ||
|
|
||
| </LinearLayout> |
There was a problem hiding this comment.
Check the comment above https://github.com/owncloud/android/pull/4794/changes#r2994418254
| <TextView | ||
| android:id="@+id/password_policy_min_characters_icon" | ||
| android:layout_width="wrap_content" | ||
| android:layout_height="wrap_content"/> |
There was a problem hiding this comment.
Why is this a TextView if the content is an icon? same for all below ⬇️
There was a problem hiding this comment.
Yeah, they should be ImageView elements... Thanks! Changed 😄
| android:layout_width="match_parent" | ||
| android:layout_height="wrap_content"/> | ||
|
|
||
| </LinearLayout> |
There was a problem hiding this comment.
Seems to be repetition there (one LinearLayout for every policy, just changing ids) . Is there any way to refactor, for example, using includes somehow ? or the different ids in every Layout makes it not posible?
| import com.owncloud.android.lib.resources.Service | ||
|
|
||
| interface LinksService: Service { | ||
| fun addLink(spaceId: String, displayName: String, type: String, expirationDate: String?, password: String?): RemoteOperationResult<Unit> |
There was a problem hiding this comment.
Question: could the type be OCLinkType instead of String?
3 participants
Related Issues
App: #4753
ReleaseNotesViewModel.ktcreating a newReleaseNote()with String resources (if required)EXTRA: Some content descriptions have been refactored (removed redundant
button) in order to avoid repetitions inTalkback. These strings are related to theCancelandCreatebuttons, displayed in some dialogs:Cancel-> Set password dialog, create space dialog and set icon dialogCreate-> Create space dialogQA