Sentaroh / SMBSync2

This application performs file synchronization between an Android device and a PC/NAS via wireless LAN using SMB v1/v2/v3 protocol.
MIT License
262 stars 49 forks source link

History List is not always updated #174

Closed PhilZ-cwm6 closed 4 years ago

PhilZ-cwm6 commented 4 years ago

Hi,

Pull request: https://github.com/Sentaroh/SMBSync2/pull/173

Steps to reproduce:

Result: The History entry is not shown until we touch the screen

I pushed a pull request but I am not sure if this is the best way to fix it or better in SyncThread.addHistoryList(...)

Best regards

PhilZ-cwm6 commented 4 years ago

More commits on this patch:

  1. Add sequence numbers in SyncTask like in History lists
    • Add ")" separator to existing History sequence number
    • Add sequence number to SyncTask List because order of tasks matters when starting them with AutoSync button Also, when moving a sync task up/down, we can follow its order in an easier visual way
Screenshot_1601390060 Screenshot_1601389923
  1. Fix separator in History path error message

    Screenshot_1601390714
  2. Fix bug where FastSrollBar is not re-enabled for Messages tab at end of sync After a sync, the FastScroll bar was not re-enabled at end of sync until app restart

  3. History: add FastScroll bar + change up/down buttons to PG-UP and PG-Down This last one is really useful to check last days history on a few pages. Much more visually confortable than a scroll We do not need jump top/bottom in History like in Messages + FastScroll bar makes it instant to access top and bottom

Thank you and best regards

PhilZ-cwm6 commented 4 years ago

A last commit to enhance a bit more the Up/Down buttons of History

Below is a video to show the additional confort that a page scroll can give compared to a touch scroll: For reading through the last 3-5 pages of Messages or History, this is by far more comfortable than touch scroll to follow with the eyes

History with the changed Buttons: https://youtu.be/fxNb9G18yjU

Messages with the original buttons: https://youtu.be/BO6YBYx_eo0

As you see, the reading confort of a long history is much better with the buttons. This patch keeps the scroll/touch gui while adding better functional scroll buttons.

Hope you can test and think at it before deciding. If ok, I can add patch to Messages or you can do if you have code optimizations to add

Many thanks and Best regards

PhilZ-cwm6 commented 4 years ago

Last technical notes

Best regards

PhilZ-cwm6 commented 4 years ago

Hi, I updated the patch with a generic class onLongTouch The root code is same using the recommended repeat Runner However, using a class makes it much simpler to call on any view where we need onLongTouch repeatable action

I also implemented it in Messages tab: the readability with eyes is really much more comfortable compared to touch scroll on long Messages and History pages because we do not have the blur effect of touch scroll transitions

To summarize it, the result of this patch :

The implementation is following Android app standards (no double tap, using recommended touch implementation)

Hope it is ok for you

Best regards

Sentaroh commented 4 years ago

Thank you.

In addition to your changes, the following changes have been made 1.MoveToTop and MoveToBottom buttons have been restored 2.Added Scroll up/down buttons. Scroll amount is 1.(Because it is difficult to get them in the same position Down and Up.) image

Sorry, following commit was not merged. Please don't update as I will take in the features on my end. 1.Add Page UP/Down and Fast Scroll to Messages context buttons 2.Implement simpler and more generic onLongTouch handler 3.Enable back context info message for move buttons 4.Remove unused message labels from templates

Best regards.

PhilZ-cwm6 commented 4 years ago

Hi,

Thank you a lot for taking up with the code Adding dedicated scroll up/down buttons is a great idea

I read by email only your first comment, before you edited it and where you ask me to rebase. So I did not see early that you were working on it.

However, since I just notice it, I did not modify previous pull request and I created a new one with the changes: pull request: https://github.com/Sentaroh/SMBSync2/pull/175

I am a visually impaired guy. The new scroll mode is really convenient and comfortable for fast reading of History/Message pages without much scroll and blur caused by the scroll. Please let me know if you see some bugs in the scroll code

I am holding any commits until you are done

Note: can you make the move/scroll buttons be hidden when scroll is not needed (getFirstVisiblePosition == 0 && getLastVisiblePosition == list.size-1 && first_item is visible && last_item is visible)

Thank you again and best regards

PhilZ-cwm6 commented 4 years ago

Thank you for the merging and enhancements.

I pushed a commit with a few bug fixes

When we read on phone, we usually need a page by page view, not a line by line. The item by item scroll mode in Messages will be less useful in daily use because people would better touch scroll a whole page to get the next infos. I really hope you merge the page by page scroll. It is needed when we must read through a long last sync list

Here's the result on a video showing the effect on a simulated very long items too Video link: https://youtu.be/LgcxW7V5N-0

I click each time on the view that will be kept on page up / page down. It shows that the new code gives an exact scroll on items that do not take whole screen while it gives a readable mode with 3 lines context on very long items

We can have very long items on some error messages displayed through ThrowException

Best regards

Sentaroh commented 4 years ago

Thank you.

The following is merged.

The following is not merged.

Best regards.

PhilZ-cwm6 commented 4 years ago

I am sorry to insist, but please look at it this other way: In your last "fixed" pic, the bottom part is not aligned and partially invisible while in my version it was made on purpose the inverse of the scroll down: bottom part visible while top is not aligned.

In the reading mode of apps on browsers, or Word/Text editors and also on Tablet Readers it works this way:

When you scroll down, the reading logic is the normal way: you read from top to bottom: last item is partially visible. On page down, the last partially visible item becomes visible and completely aligned on top

It is not a bug but by design to respect the common implementation of Page UP and Page Down buttons Here's a video on how it works in Microsoft Word when you scroll up: https://youtu.be/bWwMhRKkcOU

As you see, the top line is partially visible and when you scroll up, it becomes visible on the bottom with one line of context

Hope you adjust it because a scroll one by one is not really readable and if you inverse the align logic it won't be useful. It is for a reason all text editors that are known work this way. I am a professional in ergonomics for office by the way.

Try again my code to read the Messages of a last sync, from the bottom to the top and you'll see it is logic like when you scroll with finger because top item is never aligned when you start the view and scroll up

For the labels toasts: yes, I agree they are not needed at all and should be removed

Thank you for your time and best regards

Edit 1: I just looked at your code: using a scroll amount of 3 in Messages, you will miss some messages on long paths + error codes on small screens. Also, on each scroll (amount 1 and 3 or any amount), you have 80% of the page filled with info you already read and you have to click many times to get all new info on a single page. If you increase amount, you miss some info

In the patch I pushed, it will follow MS Office like in the video and on each page you get the info that was missing on previous page and you never miss a line of messages from page to page

Best regards

Sentaroh commented 4 years ago

I know that different individuals have different scrolling preferences, so why not add a page-by-page scrolling button? image

Best regards.

PhilZ-cwm6 commented 4 years ago

Oh, that's really even a better idea. Would be great if you make it that way and would perfectly match my scroll preferences.

A side note: I just noticed that we end up looking for the "missing" button when they disappear at end of a top/down scroll. What if you only hide ALL buttons at once, when the list is not scrollable at all ? But that part is optional for me and I am ok if you keep them appear / disappear like implented now (they would give a hint we're at end of the list) as we get used to it after a while.

Edit 7:38: Also, maybe add some more little space between page up and page down buttons so touch do not overlap. And is it possible to increase buttons size or it would cause issues with the layout?

Thank you again for having considered my special needs in scrolling and best regards.

Sentaroh commented 4 years ago

Hi,

Pushing is done, so please take a look.

Best regards.

PhilZ-cwm6 commented 4 years ago

thank you, I will give a feedback in the next 24h best regards

PhilZ-cwm6 commented 4 years ago

I tested the new layout and code The buttons size and spacing looks really perfect for me. The look and buttons design is also superb. I like the functionality of the different scroll options.

I only have one small concern:

I suggest backing it to 100 or 150 msec. If we want faster, we have the fast scroll bar which is very useful for that.

Otherwise, that part of implementation is really nice and GUI looks good

Thank you again for this quick follow up and best regards

PhilZ-cwm6 commented 4 years ago

Sorry, I was not clear enough: Set to 100 msec was only for pageUP and pageDown

For mContextMessageButtonScrollUp and mContextMessageButtonScrollDown, you can keep them at your setting of 50 msec, it is perfect because they only scroll one by one, so visibility is good at 50 msec

Sorry again for the confusion. If you don't have time, I will push a pull request with translations once you are done changing the code in the next days

Thank you again for the follow up and best regards