PR targeting multiple issues: #1101, #1092, #1080, #1079, #1057 #1109

Merged
mmarif merged 14 commits from bottomsheets-actions-search-filters-apilimit-tags into main 2 months ago
mmarif commented 2 months ago
Owner
- [x] Closes #1101 - [x] Closes #1092 - [x] Closes #1080 - [x] Closes #1079 - [x] Closes #1057 - [x] Closes #1107 - [x] Closes #913
mmarif added this to the 4.3.0 milestone 2 months ago
mmarif added the
Enhancement
Improvement
UI/UX
labels 2 months ago
mmarif self-assigned this 2 months ago
mmarif added 1 commit 2 months ago
mmarif changed title from PR targeting multiple issues: #1101, #1092, #1080, #1079, #1057 to PR targeting multiple issues: #1101, #1092, #1080, #1079, #1057 2 months ago
mmarif added 1 commit 2 months ago
mmarif added 1 commit 2 months ago
mmarif added 1 commit 2 months ago
mmarif changed title from PR targeting multiple issues: #1101, #1092, #1080, #1079, #1057 to [WIP] PR targeting multiple issues: #1101, #1092, #1080, #1079, #1057 2 months ago
mmarif added 1 commit 2 months ago
mmarif added 1 commit 2 months ago
mmarif added 1 commit 2 months ago
mmarif added 1 commit 2 months ago
mmarif added 1 commit 2 months ago
mmarif added 1 commit 2 months ago
mmarif changed title from [WIP] PR targeting multiple issues: #1101, #1092, #1080, #1079, #1057 to PR targeting multiple issues: #1101, #1092, #1080, #1079, #1057 2 months ago
Poster
Owner

This PR is ready for testing.

This PR is ready for testing.
mmarif added 1 commit 2 months ago
6543 approved these changes 2 months ago
6543 left a comment
Collaborator

got my lgtm

got my lgtm
Collaborator

@mmarif please resolve conflicts :)

@mmarif please resolve conflicts :)
mmarif added 2 commits 2 months ago
Poster
Owner

@6543 done. :)

@6543 done. :)
qwerty287 reviewed 2 months ago
private String spinnerSelectedValue;
private Version giteaVersion;
private int maxResponseItems = 50;
private int defaultPagingNumber = 25;
Collaborator

Gitea's default for this is 30.

Gitea's default for this is 30.
mmarif marked this conversation as resolved
@Override
public void migrate(@NonNull SupportSQLiteDatabase database) {
database.execSQL("ALTER TABLE 'userAccounts' ADD COLUMN 'maxResponseItems' INTEGER NOT NULL DEFAULT 50");
database.execSQL("ALTER TABLE 'userAccounts' ADD COLUMN 'defaultPagingNumber' INTEGER NOT NULL DEFAULT 25");
Collaborator

Gitea's default for this is 30, we should adapt this.

Gitea's default for this is 30, we should adapt this.
mmarif marked this conversation as resolved
public static int getCurrentResultLimit(Context context) {
return ((BaseActivity) context).getAccount().requiresVersion("1.12") ? resultLimitNewGiteaInstances : resultLimitOldGiteaInstances;
return ((BaseActivity) context).getAccount().requiresVersion("1.15") ? ((BaseActivity) context).getAccount().getDefaultPageLimit() : ((BaseActivity) context).getAccount().getMaxPageLimit();
Collaborator

Maybe I missed something at Gitea here, but why can't we always use max limit? Is the max limit filed new in 1.15?
And BTW, support for Gitea < 1.14 is dropped, so this condition can be safely removed.

Maybe I missed something at Gitea here, but why can't we always use max limit? Is the max limit filed new in 1.15? And BTW, support for Gitea < 1.14 is dropped, so this condition can be safely removed.
Poster
Owner

Let's keep the check for now. We can remove it in later releases.

Let's keep the check for now. We can remove it in later releases.
mmarif marked this conversation as resolved
android:gravity="center"
app:layout_alignSelf="flex_start"
app:drawableTopCompat="@drawable/ic_browser"
android:text="@string/isOpen"
Collaborator

That's too short. "Open" as text can say anything. At least, the browser must appear here in the string (also in all the other usages).

That's too short. "Open" as text can say anything. At least, the browser must appear here in the string (also in all the other usages).
Poster
Owner

Even though the icon is self explanatory, but for the context I have added browser to it.

Even though the icon is self explanatory, but for the context I have added browser to it.
mmarif marked this conversation as resolved
android:visibility="gone"
android:textSize="16sp" />
<TextView
Collaborator

Not related, but can you hide this view if the repo is archived? There is a condition in the BottomSheet fragment, just add it here.

Not related, but can you hide this view if the repo is archived? There is a condition in the BottomSheet fragment, just add it here.
Poster
Owner

It already have this android:visibility="gone". Am I missing something? Or you mean a check in the bottomsheet?

It already have this `android:visibility="gone"`. Am I missing something? Or you mean a check in the bottomsheet?
Poster
Owner

Added a check.

Added a check.
mmarif marked this conversation as resolved
android:textColor="?attr/primaryTextColor"
android:textSize="16sp" />
<TextView
Collaborator

Not related, but can you hide this view if the repo is archived? There is a condition in the BottomSheet fragment, just add it here.

Not related, but can you hide this view if the repo is archived? There is a condition in the BottomSheet fragment, just add it here.
mmarif marked this conversation as resolved
android:textColor="?attr/primaryTextColor"
android:textSize="16sp" />
<TextView
Collaborator

Not related, but can you hide this view if the repo is archived? There is a condition in the BottomSheet fragment, just add it here.

Not related, but can you hide this view if the repo is archived? There is a condition in the BottomSheet fragment, just add it here.
Collaborator

This actually needs a check if the head repo is archived, the head repo is already requested in the issue detail activity, just store and check the value here.

This actually needs a check if the head repo is archived, the head repo is already requested in the issue detail activity, just store and check the value here.
mmarif marked this conversation as resolved
android:gravity="center"
app:layout_alignSelf="flex_start"
app:drawableTopCompat="@drawable/ic_branch"
android:text="@string/menuDeleteText"
Collaborator

I'm not sure if this is a good idea. Only "delete" is not enough - the icon doesn't say everything. In case of a repository, that's fine because most will now the repo icon, but the branch icon isn't that clear. I understand your thoughts here, but this is not really good in terms of accessibility.

I'm not sure if this is a good idea. Only "delete" is not enough - the icon doesn't say everything. In case of a repository, that's fine because most will now the repo icon, but the branch icon isn't that clear. I understand your thoughts here, but this is not really good in terms of accessibility.
mmarif marked this conversation as resolved
android:textColor="?attr/primaryTextColor"
android:textSize="16sp" />
</com.google.android.flexbox.FlexboxLayout>
Collaborator

What about share/open in browser/copy here?

What about share/open in browser/copy here?
Poster
Owner

As this part is more of a diverse, adding a specific copy will not suffice. In repo it's fine to say Repository as we have repo specific things like Settings. But here I am not sure. If we want to add a header copy, it should be more generic to cover all the used items.

As this part is more of a diverse, adding a specific copy will not suffice. In repo it's fine to say Repository as we have repo specific things like Settings. But here I am not sure. If we want to add a header copy, it should be more generic to cover all the used items.
<?xml version="1.0" encoding="utf-8"?>
<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
Collaborator

This if fine for me now, but later I would like to change it. I like the Octodroid way, I'll try to add a screenshot (can't add it on the reviews).

This if fine for me now, but later I would like to change it. I like the Octodroid way, I'll try to add a screenshot (can't add it on the reviews).
Collaborator
https://codeberg.org/gitnex/GitNex/pulls/1109#issuecomment-438960
Collaborator

Screenshot of Octodroids PR "dashboard". I only refer to the branch views, not the general tabbed layout etc.

Screenshot of Octodroids PR "dashboard". I only refer to the branch views, not the general tabbed layout etc.
Poster
Owner

@qwerty287 Most of your ideas are implemented.

  • What about share/open in browser/copy here?

As commented by me, if you have more generic copy(text) we can add that. I am out of luck for it. :)

  • This if fine for me now, but later I would like to change it. I like the Octodroid way, I'll try to add a screenshot (can't add it on the reviews).

When I was implementing it I thought of inline usage like other items we currently have, but then I thought if what we have more info to add in the future. So instead of cluterring the main view, I preferred to have info icon where we add stuff and expand it later.

Octodrioid implementation is fine too. If you think we should be adding stuff(more) in the main view. I can change it or you can change it in this PR.

@qwerty287 Most of your ideas are implemented. - What about share/open in browser/copy here? As commented by me, if you have more generic copy(text) we can add that. I am out of luck for it. :) - This if fine for me now, but later I would like to change it. I like the Octodroid way, I'll try to add a screenshot (can't add it on the reviews). When I was implementing it I thought of inline usage like other items we currently have, but then I thought if what we have more info to add in the future. So instead of cluterring the main view, I preferred to have info icon where we add stuff and expand it later. Octodrioid implementation is fine too. If you think we should be adding stuff(more) in the main view. I can change it or you can change it in this PR.
mmarif added 1 commit 2 months ago
qwerty287 approved these changes 2 months ago
qwerty287 left a comment
Collaborator

If the review changes are working (didn't test), LGTM.

If the review changes are working (didn't test), LGTM.
Collaborator

Octodrioid implementation is fine too. If you think we should be adding stuff(more) in the main view. I can change it or you can change it in this PR.

Let's keep it for now I'll review it in 4.3.0 - I never worked with a dialog-based system, so I'll try first. If it doesn't work well for anybody, we can change it later.

> Octodrioid implementation is fine too. If you think we should be adding stuff(more) in the main view. I can change it or you can change it in this PR. Let's keep it for now I'll review it in 4.3.0 - I never worked with a dialog-based system, so I'll try first. If it doesn't work well for anybody, we can change it later.
mmarif merged commit c67b3c178f into main 2 months ago
mmarif deleted branch bottomsheets-actions-search-filters-apilimit-tags 2 months ago

Reviewers

6543 approved these changes 2 months ago
qwerty287 approved these changes 2 months ago
The pull request has been merged as c67b3c178f.
Sign in to join this conversation.
No reviewers
No Milestone
No project
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.