Increasing usability and design of files diff. #413

Merged
mmarif merged 42 commits from :diff-cleaner into master 2020-04-28 12:39:46 +00:00
Member

Closes #397

Todo

  • Include code commenting

  • (To cite code, click on the line(s) you want to cite and (if you're ready) long press on any already cited line.)

  • Support dark mode.

Summary

  • Much better design of diff.
  • Allow for citation of code
  • Sorting cited code in order of line numbers
  • Limitation of how many lines can be actually displayed.
  • Moving ProgressBar from top to center
  • Fixing bug (of previous implementation) where code would be duplicated due to the recyclerview.
  • Improving proportions of 'close'-buttons.
Closes #397 #### Todo - [x] Include code commenting - (To cite code, click on the line(s) you want to cite and (if you're ready) long press on any already cited line.) - [x] Support dark mode. #### Summary - **Much better design of diff.** - **Allow for citation of code** - **Sorting cited code in order of line numbers** - Limitation of how many lines can be actually displayed. - Moving ProgressBar from top to center - Fixing bug (of previous implementation) where code would be duplicated due to the recyclerview. - Improving proportions of 'close'-buttons.
opyale added the
Enhancement
label 2020-04-15 01:06:45 +00:00
opyale self-assigned this 2020-04-15 01:06:45 +00:00
opyale added the
UI/UX
label 2020-04-15 01:11:50 +00:00
opyale changed title from [WIP] Increasing usability and design of files diff. to Increasing usability and design of files diff. 2020-04-15 04:49:51 +00:00
opyale added the
LGTM-need
label 2020-04-15 04:50:06 +00:00
opyale added the
Feature
label 2020-04-15 04:57:13 +00:00
mmarif added this to the 3.0.0 milestone 2020-04-16 18:28:31 +00:00
mmarif requested changes 2020-04-16 19:01:27 +00:00
Dismissed
@ -6,6 +6,7 @@
<dimen name="tooltipCornor">5dp</dimen>
<dimen name="fab_margin">16dp</dimen>
<dimen name="close_size">23dp</dimen>

Please retain the original size, as 23 is too tiny on screen with higher resolution.

Please retain the original size, as 23 is too tiny on screen with higher resolution.

Also better name would be close_button_size.

Also better name would be `close_button_size`.
Author
Member

Really? It looks absolutly fine in your screenshot.

Really? It looks absolutly fine in your screenshot.

That is not high resolution phone.

Also the size was set to match the back button, hamburger menu etc. Compare to them this one at this size is small now.

That is not high resolution phone. Also the size was set to match the back button, hamburger menu etc. Compare to them this one at this size is small now.

I have just did a quick check and maybe 30 is a bit big. 26 does match the rest. A sweet spot.

I have just did a quick check and maybe 30 is a bit big. 26 does match the rest. A sweet spot.
Author
Member

Sounds great.

Sounds great.
@ -118,6 +118,8 @@
<string name="orgCreatedError">Something went wrong, please try again</string>
<string name="orgExistsError">Organization already exists</string>
<string name="binaryFileError">Binary files cannot be shown.</string>

Binary files cannot be shown. could be Binary files are not supported yet

`Binary files cannot be shown.` could be `Binary files are not supported yet`
Owner

Here are the actual comments:

  • Use the default fonts as used by the app.

  • Padding should be added to left and right as it really touches the screen edges.

  • How does the citation work? I clicked few lines, and hold left mouse button and it crashed the app.

  • If file occupy less space than visible screen size, the layout should be at the top. See screenshot.

  • Please retain the old filename for FilesDiffAdapter.java.

  • You removed recyclerview for listview, any benefits?

  • Please use colors from colors.xml.

As we are on it, it would be nice to have proper color scheme so different themes can have nice look. It's easy, just the backgrounds and colors need to be adjusted.

Here are the actual comments: - Use the default fonts as used by the app. - Padding should be added to left and right as it really touches the screen edges. - How does the citation work? I clicked few lines, and hold left mouse button and it crashed the app. - If file occupy less space than visible screen size, the layout should be at the top. See screenshot. - Please retain the old filename for `FilesDiffAdapter.java`. - You removed recyclerview for listview, any benefits? - Please use colors from colors.xml. As we are on it, it would be nice to have proper color scheme so different themes can have nice look. It's easy, just the backgrounds and colors need to be adjusted.
Author
Member

Use the default fonts as used by the app.

This is intended and would'nt fit otherwise.
Monospace is used for source code, because it makes it much easier to read.

Padding should be added to left and right as it really touches the screen edges.

I may have a look at it, but i think too much padding may be unwanted, because the code itself often includes much empty space.

How does the citation work? I clicked few lines, and hold left mouse button and it crashed the app.

This shouldnt happen. Are you able to send me any stacktraces and additional information (repo, pr, etc.) for further investigation?

If file occupy less space than visible screen size, the layout should be at the top. See screenshot.

This was not intended and will be fixed.

You removed recyclerview for listview, any benefits?

Please scroll through the diff in the current release. Some textviews will be duplicated and are placed in positions, where they should'nt be. Thats the reason why i am using a listview in this specific case.

Please retain the old filename for FilesDiffAdapter.java.

Please use colors from colors.xml.

I will do that.

> Use the default fonts as used by the app. This is intended and would'nt fit otherwise. Monospace is used for source code, because it makes it much easier to read. > Padding should be added to left and right as it really touches the screen edges. I may have a look at it, but i think too much padding may be unwanted, because the code itself often includes much empty space. > How does the citation work? I clicked few lines, and hold left mouse button and it crashed the app. This shouldnt happen. Are you able to send me any stacktraces and additional information (repo, pr, etc.) for further investigation? > If file occupy less space than visible screen size, the layout should be at the top. See screenshot. This was not intended and will be fixed. > You removed recyclerview for listview, any benefits? Please scroll through the diff in the current release. Some textviews will be duplicated and are placed in positions, where they should'nt be. Thats the reason why i am using a listview in this specific case. > Please retain the old filename for `FilesDiffAdapter.java`. > Please use colors from colors.xml. I will do that.
Owner

This is intended and would’nt fit otherwise. Monospace is used for source code, because it makes it much easier to read.

Ok. Hope @6543 is also ok with keeping this.

I may have a look at it, but i think too much padding may be unwanted, because the code itself often includes much empty space.

Remember some phones have screen egdes, adding padding just occupy the screen in front. Also it just does not look nice that way either by touching the phone edges. Looks rough.

This shouldnt happen. Are you able to send me any stacktraces and additional information (repo, pr, etc.) for further investigation?

Here

android.util.AndroidRuntimeException: Calling startActivity() from outside of an Activity  context requires the FLAG_ACTIVITY_NEW_TASK flag. Is this really what you want?
        at android.app.ContextImpl.startActivity(ContextImpl.java:912)
        at android.app.ContextImpl.startActivity(ContextImpl.java:888)
        at android.content.ContextWrapper.startActivity(ContextWrapper.java:379)
        at org.mian.gitnex.adapters.FilesDiffLinesAdapter.lambda$getView$1$FilesDiffLinesAdapter(FilesDiffLinesAdapter.java:194)
        at org.mian.gitnex.adapters.-$$Lambda$FilesDiffLinesAdapter$PEidmkTu-_e6hDJlw7plo4UHFTo.onLongClick(Unknown Source:2)
        at android.view.View.performLongClickInternal(View.java:6677)
        at android.view.View.performLongClick(View.java:6635)
        at android.widget.TextView.performLongClick(TextView.java:11340)
        at android.view.View.performLongClick(View.java:6653)
        at android.view.View$CheckForLongPress.run(View.java:25850)
        at android.os.Handler.handleCallback(Handler.java:873)
        at android.os.Handler.dispatchMessage(Handler.java:99)
        at android.os.Looper.loop(Looper.java:193)
        at android.app.ActivityThread.main(ActivityThread.java:6669)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:493)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:858)

Please scroll through the diff in the current release. Some textviews will be duplicated and are placed in positions, where they should’nt be. Thats the reason why i am using a listview in this specific case.

It could be a flaw in the implentation not necessarly recyclerview issue. But as you already used listview, it's ok now.

Also from my screenshot above, the background color should take whole line into account like how it was in previous implementation or how it look like in web.

> This is intended and would’nt fit otherwise. Monospace is used for source code, because it makes it much easier to read. Ok. Hope @6543 is also ok with keeping this. > I may have a look at it, but i think too much padding may be unwanted, because the code itself often includes much empty space. Remember some phones have screen egdes, adding padding just occupy the screen in front. Also it just does not look nice that way either by touching the phone edges. Looks rough. > This shouldnt happen. Are you able to send me any stacktraces and additional information (repo, pr, etc.) for further investigation? Here ``` android.util.AndroidRuntimeException: Calling startActivity() from outside of an Activity context requires the FLAG_ACTIVITY_NEW_TASK flag. Is this really what you want? at android.app.ContextImpl.startActivity(ContextImpl.java:912) at android.app.ContextImpl.startActivity(ContextImpl.java:888) at android.content.ContextWrapper.startActivity(ContextWrapper.java:379) at org.mian.gitnex.adapters.FilesDiffLinesAdapter.lambda$getView$1$FilesDiffLinesAdapter(FilesDiffLinesAdapter.java:194) at org.mian.gitnex.adapters.-$$Lambda$FilesDiffLinesAdapter$PEidmkTu-_e6hDJlw7plo4UHFTo.onLongClick(Unknown Source:2) at android.view.View.performLongClickInternal(View.java:6677) at android.view.View.performLongClick(View.java:6635) at android.widget.TextView.performLongClick(TextView.java:11340) at android.view.View.performLongClick(View.java:6653) at android.view.View$CheckForLongPress.run(View.java:25850) at android.os.Handler.handleCallback(Handler.java:873) at android.os.Handler.dispatchMessage(Handler.java:99) at android.os.Looper.loop(Looper.java:193) at android.app.ActivityThread.main(ActivityThread.java:6669) at java.lang.reflect.Method.invoke(Native Method) at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:493) at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:858) ``` > Please scroll through the diff in the current release. Some textviews will be duplicated and are placed in positions, where they should’nt be. Thats the reason why i am using a listview in this specific case. It could be a flaw in the implentation not necessarly recyclerview issue. But as you already used listview, it's ok now. Also from my screenshot above, the background color should take whole line into account like how it was in previous implementation or how it look like in web.
Author
Member

@mmarif I have addressed many of your concerns in a new commit (colors are following). Are you able to test, if citation works now?

@mmarif I have addressed many of your concerns in a new commit (colors are following). Are you able to test, if citation works now?
opyale requested review from mmarif 2020-04-17 12:57:02 +00:00
Owner

So far so good. Noticed few things which can be addressed along the way.

Citation works now, but if I cite and go to the reply section the start cursor is inside the code blocks(2nd line). Would be nice to have it in 1st line or below the code block.

The line background colors I guess you are still working on.

id/headerFileName textview, id/header and View colors are still hardcoded.

So far so good. Noticed few things which can be addressed along the way. Citation works now, but if I cite and go to the reply section the start cursor is inside the code blocks(2nd line). Would be nice to have it in 1st line or below the code block. The line background colors I guess you are still working on. `id/headerFileName` textview, `id/header` and View colors are still hardcoded.
Owner

Would be nice to have it in 1st line or below the code block.

This one is more generic issue though not related to this PR.

> Would be nice to have it in 1st line or below the code block. This one is more generic issue though not related to this PR.
Author
Member

Would be nice to have it in 1st line or below the code block.

7dae652bf6

> Would be nice to have it in 1st line or below the code block. 7dae652bf614ded237ddf91e0650bf011b8f9318
Author
Member

@mmarif Colors are still work in progress..

@mmarif Colors are still work in progress..
Owner

Seems like you guys like switch more than if. 😆

Seems like you guys like switch more than if. :laughing:
Member

This is intended and would’nt fit otherwise. Monospace is used for source code, because it makes it much easier to read.

Ok. Hope @6543 is also ok with keeping this.

I'm ok with that but we will have some rewirte if we got a better api

for the collor: It would be good if the green woud be litle more green and the red more red

It also would be good to put colors into the theme so it changes based on witch theme you have -> white background on dark mode is - uggly

also can we add a divider between code blocks in same file - drawed suggestion (see atfachment)

> > This is intended and would’nt fit otherwise. Monospace is used for source code, because it makes it much easier to read. > > Ok. Hope @6543 is also ok with keeping this. I'm ok with that but we will have some rewirte if we got a better api for the collor: It would be good if the green woud be litle more green and the red more red It also would be good to put colors into the theme so it changes based on witch theme you have -> white background on dark mode is - uggly also can we add a divider between code blocks in same file - drawed suggestion (see atfachment)
Member

ok attachments are broken so ..:

pic

ok attachments are broken so ..: ![pic](https://cloud.obermui.de/apps/files_sharing/publicpreview/sF5xr5yee9bmqCC?x=1000&y=550&a=true&file=paint-it.png&scalingup=0)
Owner

for the collor: It would be good if the green woud be litle more green and the red more red

I am for keeping it the same as the web ui. Last time was like that.

It also would be good to put colors into the theme so it changes based on witch theme you have -> white background on dark mode is - uggly

Maybe in another PR?

> for the collor: It would be good if the green woud be litle more green and the red more red I am for keeping it the same as the web ui. Last time was like that. > It also would be good to put colors into the theme so it changes based on witch theme you have -> white background on dark mode is - uggly Maybe in another PR?
Member

Maybe in another PR?

I'm fine with that too

> Maybe in another PR? I'm fine with that too
Author
Member

Great, but let me still have a look at the attachments issue @6543 had. Can you tell me which pull request you tried to access and maybe some hints for reproduction?

Great, but let me still have a look at the attachments issue @6543 had. Can you tell me which pull request you tried to access and maybe some hints for reproduction?
Owner

but let me still have a look at the attachments issue @6543 had

I feel he is referring to gitea.com attachements which are kinda broken after update.

> but let me still have a look at the attachments issue @6543 had I feel he is referring to gitea.com attachements which are kinda broken after update.
Member

@mmarif yes

@opyale pleace "update" this branch - I cant since its portected

@mmarif yes @opyale pleace "update" this branch - I cant since its portected
Author
Member

@6543 Done.

@6543 Done.
opyale changed title from Increasing usability and design of files diff. to [WIP] Increasing usability and design of files diff. 2020-04-26 18:08:49 +00:00
Author
Member

@mmarif @6543 Added support for dark mode.

@mmarif @6543 Added support for dark mode.
opyale changed title from [WIP] Increasing usability and design of files diff. to Increasing usability and design of files diff. 2020-04-26 22:52:48 +00:00
Member

@opyale can you "Update" this branch ?

@opyale can you "Update" this branch ?
Author
Member

@6543 Did it already.

@6543 Did it already.
6543 approved these changes 2020-04-27 01:27:32 +00:00
Dismissed
Member

I'll have a look at the crashes after VersionRefactor

I'll have a look at the crashes after VersionRefactor
Author
Member

@mmarif @6543 I would suggest also targeting the 2.5.1 milestone, because these changes do also include minor bug fixes (like code duplication).

@mmarif @6543 I would suggest also targeting the 2.5.1 milestone, because these changes do also include minor bug fixes (like code duplication).
Member

@code dedublication is no bug.

If you fixed what are those?

@code dedublication is no bug. If you fixed what are those?
Author
Member

@6543 Duplication and random scrambling of code should be considered as a bug. I've already sent you a screenshot where XML glitched into some java code.

@6543 Duplication and random scrambling of code should be considered as a bug. I've already sent you a screenshot where XML glitched into some java code.
Owner

I would suggest also targeting the 2.5.1 milestone, because these changes do also include minor bug fixes (like code duplication).

Seems this is already settled with another PR and that's how it has to be done. This PR even has fixes cannot be part of minor bug fix releases.

> I would suggest also targeting the 2.5.1 milestone, because these changes do also include minor bug fixes (like code duplication). Seems this is already settled with another PR and that's how it has to be done. This PR even has fixes cannot be part of minor bug fix releases.
mmarif requested changes 2020-04-28 05:47:26 +00:00
Dismissed
@ -58,3 +57,3 @@
android:id="@+id/listView"
android:layout_width="match_parent"
android:layout_height="match_parent"
android:background="?attr/primaryBackgroundColor"

Please keep this and remove android:background="@color/white" for theme changes. It will also remove the white background in dark theme.

Please keep this and remove `android:background="@color/white"` for theme changes. It will also remove the white background in dark theme.
Author
Member

Oops. I did also notice this, but forgot about it...

Oops. I did also notice this, but forgot about it...
opyale marked this conversation as resolved
Owner

Nice work @opyale

Three things I noticed can be improved(one I mentioned before).

  1. Check the screenshot, the white background. It should be the color background used for that specific line(gree, red etc) extend to the end of the line.

  2. For BIN(images etc) the green, red(small change color indicators) should be hidden.

  3. This is optional but nice to have if you have time. The spacing is less and the files looks very cluttered. I would give them some breathing space between each file. If not possible right now, I will look into it after merge this PR.

https://cloud.swatian.com/s/QtJWYbCCg2rQLZ4

After fixing 1, 2 and the background for listview, we are ready to merge this.

Nice work @opyale Three things I noticed can be improved(one I mentioned before). 1. Check the screenshot, the white background. It should be the color background used for that specific line(gree, red etc) extend to the end of the line. 2. For BIN(images etc) the green, red(small change color indicators) should be hidden. 3. **This is optional** but nice to have if you have time. The spacing is less and the files looks very cluttered. I would give them some breathing space between each file. If not possible right now, I will look into it after merge this PR. https://cloud.swatian.com/s/QtJWYbCCg2rQLZ4 After fixing 1, 2 and the background for listview, we are ready to merge this.
Author
Member

@mmarif I have addressed (almost) all of your concerns in my last commits.

This is optional but nice to have if you have time. The spacing is less and the files looks very cluttered. I would give them some breathing space between each file. If not possible right now, I will look into it after merge this PR.

Honestly, the goal was to actually make everything more compact and "cluttered", because especially when there are many lines of code and many inidividual files, space on the users screen gets really valueable (and especially on small screens).

@mmarif I have addressed (almost) all of your concerns in my last commits. > This is optional but nice to have if you have time. The spacing is less and the files looks very cluttered. I would give them some breathing space between each file. If not possible right now, I will look into it after merge this PR. Honestly, the goal was to actually make everything more compact and "cluttered", because especially when there are many lines of code and many inidividual files, space on the users screen gets really valueable (and especially on small screens).
Owner

Honestly, the goal was to actually make everything more compact and “cluttered”, because especially when there are many lines of code and many inidividual files, space on the users screen gets really valueable (and especially on small screens).

We will discuss this another time. You can skip this.

> Honestly, the goal was to actually make everything more compact and “cluttered”, because especially when there are many lines of code and many inidividual files, space on the users screen gets really valueable (and especially on small screens). We will discuss this another time. You can skip this.
mmarif approved these changes 2020-04-28 12:38:57 +00:00
Dismissed
mmarif added
LGTM-done
and removed
LGTM-need
labels 2020-04-28 12:39:15 +00:00
mmarif referenced this issue from a commit 2020-04-28 12:39:43 +00:00
Increasing usability and design of files diff. (#413) Applying sizes. Additional changes. First changes. Merge branch 'master' into diff-cleaner Fixing formatting. Merge branch 'master' into diff-cleaner Final changes for working with custom themes. Merge remote-tracking branch 'remotes/main/master' into diff-cleaner First changes for working with custom themes. Merge branch 'master' into diff-cleaner Merge branch 'master' into diff-cleaner Merge branch 'master' into diff-cleaner Merge branch 'master' into diff-cleaner Merge branch 'master' into diff-cleaner Adding custom COLOR_FONT. Even smaller cleanups. Merge remote-tracking branch 'remotes/main/master' into diff-cleaner Small cleanups. Fixing bug and adding maximum line limit. Adding option to set cursor to end and small cleanup. First aid. Merge branch 'master' into diff-cleaner Merge branch 'master' into diff-cleaner Merge branch 'master' into diff-cleaner Few improvements. Performance improvements and cleanups. Minor improvements (code in order) and many bug fixes Bug fix. Combining cited code. Adding code commenting option. Renaming list_files_diffs_new to list_files_diffs Moving ProcessBar into center Increasing performance. Applying size to all icons globally. Removing another unused file. Merge remote-tracking branch 'remotes/main/master' into diff-cleaner Removing unused files. Changing size of 'close'-button. Major changes concerning design and bug fixes. Temporary save point. 2 1 Co-authored-by: opyale <example@example.com> Reviewed-on: https://gitea.com/gitnex/GitNex/pulls/413 Reviewed-by: 6543 <6543@noreply.gitea.io> Reviewed-by: M M Arif <mmarif@swatian.com>
mmarif merged commit e45dc4b311 into master 2020-04-28 12:39:44 +00:00
This repo is archived. You cannot comment on pull requests.
No reviewers
No Milestone
No Assignees
3 Participants
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: gitnex/GitNex#413
No description provided.