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

Change grayscale tiff export toast warning to dt_print #16611

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

victoryforce
Copy link
Collaborator

The toast message simply did not work due to the fact that & in the text was interpreted as the beginning of the markup entity and this led to a parsing error. Even after fixing this, we will not see this message when exporting a single image (or for the last image if multiple images are exported), as it is immediately overwritten by the following text, the export statistics.

Also, this warning is not serious enough to issue in the program interface, so I moved it to the debug print.

@TurboGit TurboGit added this to the 4.8 milestone Apr 13, 2024
Copy link
Member

@TurboGit TurboGit left a comment

Choose a reason for hiding this comment

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

What would be the best option to me is to have a queue of toast message to be displayed and one at a time pop message from the queue, display it and wait for the needed time for user to be able to read it... Alternatively we could have some slots on screen to display messages and have them displayed on screen all together, new items displayed on the bottom and pushing currently displayed one one slot above.

I'm proposing that because there is some other occurrences of messages not readable because a second one is displayed just after the first one. So better have a proper solution for this problem.

@victoryforce : Do you think you can work on that?

@victoryforce
Copy link
Collaborator Author

What would be the best option to me is to have a queue of toast message to be displayed and one at a time pop message from the queue, display it and wait for the needed time for user to be able to read it...

Let's imagine a situation when we have to display such toasts after each exported file and have to keep them on the screen for a certain time so that the user has time to read the message. Let this time be, say, 2 seconds per message. But if we export files faster (say, in 1 second), then the message display queue will end much later than the end of the export. From a UX point of view, this is an unacceptable solution.

Alternatively we could have some slots on screen to display messages and have them displayed on screen all together, new items displayed on the bottom and pushing currently displayed one one slot above.

Better. But, again, we have to answer the main question: what to do if new messages will appear earlier than the time we allocate for the user to read? If you don't change the display time, all messages may take a long time to display after the events that triggered them have ended. This is not good. If you change the display time (dynamically, depending on the speed of the appearance of new ones!), it may not be enough for the user to be able to read something. This is also not good.

I'm proposing that because there is some other occurrences of messages not readable because a second one is displayed just after the first one. So better have a proper solution for this problem.

If there are such cases, then, indeed, it is worth thinking about how to improve the mechanism of toast messages. But, still, as far as this particular case is concerned, in my opinion, a toast message is unnecessary here, as it would rather create informational noise. So, I chose the grayscale tiff option to export black and white images. Of course, I don't expect color images to magically become monochrome when exported like this without proper processing. Therefore, warnings about this for each color image will be uninformative, not to say unwelcome. So in this case I'm changing toasts to dt_print not because it's an easier solution, but because it's the solution I think is optimal.

@TurboGit
Copy link
Member

From a UX point of view, this is an unacceptable solution.

Right, so maybe limit the queue to 3 messages (or 5) and just remove older when the queue is bigger. This would solve the long export queue and will solve the issue of multiple important messages on other parts. From my experience I think I have seen only a max of 3 successive messages that should be viewed by user.

@TurboGit
Copy link
Member

I also think we should have a button on the bottom bar to be able to open a dialog with all previous messages (possibly last 100 or 1000).

@ralfbrown ralfbrown added the scope: UI user interface and interactions label Apr 20, 2024
@TurboGit TurboGit modified the milestones: 4.8, 5.0 May 18, 2024
@TurboGit TurboGit added the feature: redesign current features to rewrite label May 18, 2024
@victoryforce
Copy link
Collaborator Author

OK, I did not make the intent of this PR clear enough.

The fact is that this warning does not make sense to show in the GUI. Once again, I repeat my explanation why:

So, I chose the grayscale tiff option to export black and white images. Of course, I don't expect color images to magically become monochrome when exported like this without proper processing. Therefore, warnings about this for each color image will be uninformative, not to say unwelcome. So in this case I'm changing toasts to dt_print not because it's an easier solution, but because it's the solution I think is optimal.

I don't know if it's worth investing resources in creating a more complex toast message output system. I can't remember any cases where it would be really useful. You may know such cases, but this is a matter of a separate RFC issue to discuss the necessity and implementation. And it is not related to this PR.

I suggest accepting this PR in 4.8, the proposed fix is clearly safe.

@TurboGit
Copy link
Member

TurboGit commented Aug 5, 2024

Now that multiple log messages are displayed on screen we could certainly close this PR. I understand that it is clear to you that selecting grayscale tiff won't affect color image but I'm not sure all users would understand this at first.

We should not spam the user with trivial and irrelevant information.
@victoryforce victoryforce force-pushed the tiff-export-change-toast-to-dtprint branch from 89f5d01 to 5a2ab78 Compare October 16, 2024 10:01
@victoryforce victoryforce changed the title Change grayscale tiff export toast warning to dtprint Change grayscale tiff export toast warning to dt_print Oct 16, 2024
@victoryforce
Copy link
Collaborator Author

Now that multiple log messages are displayed on screen we could certainly close this PR.

I strongly disagree. @TurboGit Well, please consider my following arguments.

I understand that it is clear to you that selecting grayscale tiff won't affect color image but I'm not sure all users would understand this at first.

IF this is not clear to the user, we have a much bigger problem and need to make the UI in such a way that it is clear to the user.

We have a control labeled "B&W as grayscale" with a tooltip that says "saving as grayscale will reduce the size for black and white images". Both strings clearly talk about colorless (black and white) images (underlining in these strings is mine).

I don't understand how the user can decide that this control should affect color images as well. If, in your opinion, the user can perceive the description of the control in this way, then this is a problem that should be solved by changing the description (adding "saving as grayscale does not apply to color images"?), and not generating extremely annoying UI spam that will repeat many times what can be added to the description.

@TurboGit TurboGit modified the milestones: 5.0, 5.2 Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: redesign current features to rewrite scope: UI user interface and interactions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants