Display error messages for invalid trace type #231

ankusht-work · 2021-01-20T05:14:52Z

fixes #187

Signed-off-by: Ankush Tyagi ankush.tyagi@ericsson.com

ankusht-work · 2021-01-20T05:25:18Z

With this PR, the user will get a toast notification while trying to import an invalid trace. If the user attempts to import a directory that contains some invalid traces then, in that case, it will create an experiment with a set of valid traces (if any) and will be presented with a toast notification displaying all the invalid traces within that directory.

Screenshot:

bhufmann

Looks good to me. Thanks for the contribution.

mirsky-work · 2021-01-20T20:00:46Z

viewer-prototype/src/browser/trace-viewer/trace-viewer.tsx

                        }
+                        // Display the names of all the incorrect traces.
+                        if (Array.isArray(invalidTraces) && invalidTraces.length) {
+                            this.messageService.error('Invalid trace(s): ' + invalidTraces.toString());


Since this line appears both in the if and else clauses, can it be extracted outside of the condition block? Then perhaps the whole if block can be slightly simplified.

Agree. However, with the new proposed changes, this can't be extracted out :(.

MatthewKhouzam · 2021-01-21T00:53:18Z

I want to be sure about this. from what I see, this will land us on the front page of /r/programminghumor as we have "trace loaded successfully" and "invalid trace" shown at the same time. Did I misunderstand?

ankusht-work · 2021-01-21T06:35:23Z

I want to be sure about this. from what I see, this will land us on the front page of /r/programminghumor as we have "trace loaded successfully" and "invalid trace" shown at the same time. Did I misunderstand?

Nice catch :) Should we have a warning message instead? "trace loaded successfully and have a warning message displaying all the invalid trace(s). @mirsky-work @ebugden Thoughts?

MatthewKhouzam · 2021-01-21T13:37:10Z

I want to be sure about this. from what I see, this will land us on the front page of /r/programminghumor as we have "trace loaded successfully" and "invalid trace" shown at the same time. Did I misunderstand?

Nice catch :) Should we have a warning message instead? "trace loaded successfully and have a warning message displaying all the invalid trace(s). @mirsky-work @ebugden Thoughts?

In my opinion we should have the same error message on the screen. The toaster by its nature is short-lived, and so it's not google friendly. On the screen where we have "your traces loaded fine", the user can then copy-paste the error and google/bing/duckduckgo the message.

MatthewKhouzam · 2021-01-21T15:23:38Z

I want to be sure about this. from what I see, this will land us on the front page of /r/programminghumor as we have "trace loaded successfully" and "invalid trace" shown at the same time. Did I misunderstand?

Nice catch :) Should we have a warning message instead? "trace loaded successfully and have a warning message displaying all the invalid trace(s). @mirsky-work @ebugden Thoughts?

In my opinion we should have the same error message on the screen. The toaster by its nature is short-lived, and so it's not google friendly. On the screen where we have "your traces loaded fine", the user can then copy-paste the error and google/bing/duckduckgo the message.

After thinking about it:

The error message can be :
Some Traces failed to load

error 1
error 2
....

ankusht-work · 2021-01-21T19:31:02Z

I want to be sure about this. from what I see, this will land us on the front page of /r/programminghumor as we have "trace loaded successfully" and "invalid trace" shown at the same time. Did I misunderstand?

Nice catch :) Should we have a warning message instead? "trace loaded successfully and have a warning message displaying all the invalid trace(s). @mirsky-work @ebugden Thoughts?

In my opinion we should have the same error message on the screen. The toaster by its nature is short-lived, and so it's not google friendly. On the screen where we have "your traces loaded fine", the user can then copy-paste the error and google/bing/duckduckgo the message.

After thinking about it:

The error message can be :
Some Traces failed to load

error 1

error 2

....

I like your proposal but given the MVP deadline, I suggest an alternative solution.

Show an error message in case all the imported traces are invalid.
Show a warning message instead if 'any but not all' the traces are invalid and display the list of invalid traces in the toast message.

ankusht-work · 2021-01-21T21:18:02Z

I want to be sure about this. from what I see, this will land us on the front page of /r/programminghumor as we have "trace loaded successfully" and "invalid trace" shown at the same time. Did I misunderstand?

Nice catch :) Should we have a warning message instead? "trace loaded successfully and have a warning message displaying all the invalid trace(s). @mirsky-work @ebugden Thoughts?

In my opinion we should have the same error message on the screen. The toaster by its nature is short-lived, and so it's not google friendly. On the screen where we have "your traces loaded fine", the user can then copy-paste the error and google/bing/duckduckgo the message.

After thinking about it:
The error message can be :
Some Traces failed to load

error 1

error 2

....

I want to be sure about this. from what I see, this will land us on the front page of /r/programminghumor as we have "trace loaded successfully" and "invalid trace" shown at the same time. Did I misunderstand?

Nice catch :) Should we have a warning message instead? "trace loaded successfully and have a warning message displaying all the invalid trace(s). @mirsky-work @ebugden Thoughts?

In my opinion we should have the same error message on the screen. The toaster by its nature is short-lived, and so it's not google friendly. On the screen where we have "your traces loaded fine", the user can then copy-paste the error and google/bing/duckduckgo the message.

After thinking about it:
The error message can be :
Some Traces failed to load

error 1

error 2

....

I like your proposal but given the MVP deadline, I suggest an alternative solution.

Show an error message in case all the imported traces are invalid.

Show a warning message instead if 'any but not all' the traces are invalid and display the list of invalid traces in the toast message.

I have made the changes as discussed. @MatthewKhouzam please verify.

mirsky-work · 2021-01-21T22:20:03Z

viewer-prototype/src/browser/trace-viewer/trace-viewer.tsx

+
+                    if (traces === undefined || traces.length === 0) {
+                        // All the traces are invalid. Display the error message and exit.
+                        this.messageService.error('Invalid trace(s): ' + invalidTraces.toString());


When trying to import a folder that contains no metadata files, the message will say "Invalid trace(s): ", which is weird. In case of invalidTraces.length == 0 can we display something along the lines of "No valid traces found in the selected directory"?

When trying to import a folder that contains no metadata files, the message will say "Invalid trace(s): ", which is weird. In case of invalidTraces.length == 0 can we display something along the lines of "No valid traces found in the selected directory"?

Good catch, I have added a check to see if the folder is empty (doesn't contain metadata files).

Note: There are other corner cases that we need to address, post MVP, for example (what happens if you upload a JSON file, txt file or other non-ctf traces.

MatthewKhouzam

My only issue is resolved. please handle Ilya's requests :)

fixes eclipse-cdt-cloud#187 Signed-off-by: Ankush Tyagi <ankush.tyagi@ericsson.com>

ankusht-work requested review from bhufmann and tahini January 20, 2021 05:25

bhufmann approved these changes Jan 20, 2021

View reviewed changes

bhufmann requested a review from PatrickTasse January 20, 2021 18:40

mirsky-work reviewed Jan 20, 2021

View reviewed changes

ankusht-work force-pushed the FileImportError branch from 5444d88 to d04285a Compare January 21, 2021 21:05

mirsky-work reviewed Jan 21, 2021

View reviewed changes

MatthewKhouzam approved these changes Jan 21, 2021

View reviewed changes

Display error messages for invalid trace type

b0c5d7d

fixes eclipse-cdt-cloud#187 Signed-off-by: Ankush Tyagi <ankush.tyagi@ericsson.com>

ankusht-work force-pushed the FileImportError branch from d04285a to b0c5d7d Compare January 25, 2021 18:15

Merge branch 'master' into FileImportError

3f7fbdb

bhufmann merged commit f565daf into eclipse-cdt-cloud:master Jan 25, 2021

ebugden mentioned this pull request May 20, 2021

Notify when user loads incompatible trace file #328

Closed

hriday-panchasara pushed a commit to hriday-panchasara/theia-trace-extension that referenced this pull request Nov 10, 2021

Display error messages for invalid trace type (eclipse-cdt-cloud#231)

f893411

fixes eclipse-cdt-cloud#187 Signed-off-by: Ankush Tyagi <ankush.tyagi@ericsson.com>

marcdumais-work mentioned this pull request Mar 19, 2024

Test publish: latest (v0.2.0) npm packages #1058

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display error messages for invalid trace type #231

Display error messages for invalid trace type #231

ankusht-work commented Jan 20, 2021

ankusht-work commented Jan 20, 2021

bhufmann left a comment

mirsky-work Jan 20, 2021

ankusht-work Jan 21, 2021

MatthewKhouzam commented Jan 21, 2021

ankusht-work commented Jan 21, 2021

MatthewKhouzam commented Jan 21, 2021

MatthewKhouzam commented Jan 21, 2021

ankusht-work commented Jan 21, 2021

ankusht-work commented Jan 21, 2021

mirsky-work Jan 21, 2021

ankusht-work Jan 25, 2021

MatthewKhouzam left a comment

Display error messages for invalid trace type #231

Display error messages for invalid trace type #231

Conversation

ankusht-work commented Jan 20, 2021

ankusht-work commented Jan 20, 2021

bhufmann left a comment

Choose a reason for hiding this comment

mirsky-work Jan 20, 2021

Choose a reason for hiding this comment

ankusht-work Jan 21, 2021

Choose a reason for hiding this comment

MatthewKhouzam commented Jan 21, 2021

ankusht-work commented Jan 21, 2021

MatthewKhouzam commented Jan 21, 2021

MatthewKhouzam commented Jan 21, 2021

ankusht-work commented Jan 21, 2021

ankusht-work commented Jan 21, 2021

mirsky-work Jan 21, 2021

Choose a reason for hiding this comment

ankusht-work Jan 25, 2021

Choose a reason for hiding this comment

MatthewKhouzam left a comment

Choose a reason for hiding this comment