Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(mobile): remove log message counter #6865

Merged
merged 2 commits into from Feb 24, 2024

Conversation

rovo89
Copy link
Contributor

@rovo89 rovo89 commented Feb 2, 2024

Previously, the items in the log page were numbered starting with #0 and increasing from top (newest) to bottom (oldest). Being new to the app, this confused me because I would have expected that newer messages have a higher number than older messages.

Initially, I had planned to just start counting from the bottom (i.e. the oldest message would be #0), but I guess the auto-increment ID of the message would be even better as it isn't reused after clearing the logs. That is, if a number is needed at all...

Since the numbers are no longer related to the total log count, I removed the same from the title. Instead, I added the message ID to the details page title.

@shenlong-tanwen
Copy link
Member

shenlong-tanwen commented Feb 7, 2024

I'm okay with changing the order of the logs, but using the autoIncrement ID from Isar results in the following scenario where if you open the log page immediately, it will display a very large ID until you reopen the log page and then it would be fine.

image

Can you check if you can reproduce this from your end as well?

Edit: Kindly check and handle the analyzer issue as well to make the PR checks pass

@rovo89
Copy link
Contributor Author

rovo89 commented Feb 7, 2024

Ah, I briefly saw this just once and then never again... but now that you're pointing to it, I understand where it's coming from.

I see four options:

  1. Guess the next IDs by counting further from the last known ID (but we might not have the latter, or might guess wrong).
  2. Show a placeholder (e.g. #-1 or #...).
  3. Go back to simple counting of the messages, but starting with the oldest instead of the newest one.
  4. Remove the message numbers from display because they don't really add any value. The timestamp is much more helpful to understand the order and relevance of messages. Potentially, the order of timestamp and "from" could be reversed to further highlight it. The date might also be helpful as well if it's an older message.

If you have any preference, please let me know, otherwise I'd give option 4 a shot.

@shenlong-tanwen
Copy link
Member

If you have any preference, please let me know, otherwise I'd give option 4 a shot.

I don't have an actual preference but won't 3 be actually easier to implement? We already have the total number of logs and the current index, and so, it is just a matter of taking a difference between them and displaying it instead of directly displaying the index?

@rovo89
Copy link
Contributor Author

rovo89 commented Feb 7, 2024

Removing the index completely would be even easier as it's just about deleting some (parts of) lines. 😉

@shenlong-tanwen
Copy link
Member

@alextran1502 / @fyfrey Thoughts on how we can handle the log message numbering in the log page?

@fyfrey
Copy link
Contributor

fyfrey commented Feb 8, 2024

We might just not show any number at all. Logs have a timestamp and their message+data. That should be enough.

@shenlong-tanwen
Copy link
Member

@rovo89 We can remove the numbering completely. Can you update your PR with the required change?

@rovo89
Copy link
Contributor Author

rovo89 commented Feb 17, 2024

Sure, I have already done that locally. Will try tonight if moving the timestamp to the front looks better, then I'll push it.

@rovo89
Copy link
Contributor Author

rovo89 commented Feb 17, 2024

I've change the subtitle to "at in " to make the timestamp at little bit more prominent (and because it has a pretty much stable width across messages). I have some further ideas, but I think I'll save them for another PR.

@rovo89 rovo89 changed the title fix(mobile): reverse log message numbering fix(mobile): remove log message counter Feb 17, 2024
Copy link
Member

@shenlong-tanwen shenlong-tanwen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@shenlong-tanwen
Copy link
Member

@rovo89 Can you re-base your changes over the latest main branch?

@rovo89
Copy link
Contributor Author

rovo89 commented Feb 23, 2024

Sure, will do that later today.

Previously, the items in the log page were numbered starting with `#0`
and increasing from top to bottom. Being new to the app, this confused
me because I would have expected that newer messages have a higher
number than older messages.

Removing the counter completely because it doesn't add any value -
log messages are identified by their timestamp, text and other details.
@rovo89
Copy link
Contributor Author

rovo89 commented Feb 23, 2024

Looks like @jrasm91 already rebased, thanks! I'm a bit confused though. Aren't you guys using squash merges anyway, so why rebase when there's no conflict?

@jrasm91
Copy link
Contributor

jrasm91 commented Feb 23, 2024

Looks like @jrasm91 already rebased, thanks! I'm a bit confused though. Aren't you guys using squash merges anyway, so why rebase when there's no conflict?

In this case there are github action workflow changes on main that were missing in the branch. Required actions weren't passing because they didn't exist. Rebase was to make sure the required actions actually passed with your code changes.

@alextran1502 alextran1502 merged commit 5775829 into immich-app:main Feb 24, 2024
24 checks passed
@rovo89 rovo89 deleted the log_numbering branch February 24, 2024 06:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants