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

Extend status report element #919

Merged
merged 6 commits into from
Aug 8, 2022
Merged

Conversation

devanlooches
Copy link
Contributor

Right now I added the full error inside of a stretchable and scrollable element on a right side bar.
What else would you like me to include in the status update element and also why did you want me to make it hold a vector?

Copy link
Owner

@hannobraun hannobraun left a comment

Choose a reason for hiding this comment

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

Thank you, @devanlooches!

Do you think this could be placed into a corner (lower-right? doesn't matter, maybe), and made more broad than high? Then there would be fewer line breaks in the error message.

What else would you like me to include in the status update element

No more ideas right now, but now that we have it, it's easy to add more when we need to 👍

and also why did you want me to make it hold a vector?

I figured then we could display the last few status messages, and not just the last one.

@devanlooches
Copy link
Contributor Author

devanlooches commented Aug 5, 2022

Thanks for the feedback!

Do you think this could be placed into a corner (lower-right? doesn't matter, maybe), and made more broad than high? Then there would be fewer line breaks in the error message.

We could place it there but the element is supposed to auto resize itself to the contents (I don't know why it breaks the error message). But it is also stretchable so you can extend it horizontally by dragging the edge outwards.

I figured then we could display the last few status messages, and not just the last one.

How many status messages do you think should be kept before clearing it?

@devanlooches
Copy link
Contributor Author

I could make the element completely draggable that way the user can put wherever they want.

@devanlooches devanlooches requested a review from hannobraun August 5, 2022 23:11
Copy link
Owner

@hannobraun hannobraun left a comment

Choose a reason for hiding this comment

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

Thank you for the additional commits, @devanlooches, looks good!

We could place it there but the element is supposed to auto resize itself to the contents (I don't know why it breaks the error message). But it is also stretchable so you can extend it horizontally by dragging the edge outwards.

Hmm, when I grab it at its edge, I can move it, but not resize it. After I stop pressing the left mouse button, it resets to its original position.

I could make the element completely draggable that way the user can put wherever they want.

That might be ideal! Is that easy to do?

How many status messages do you think should be kept before clearing it?

I don't know. Maybe 5?

It doesn't really matter. Once the mechanism is there to store/display multiple messages and limit them to a certain number, it's easy to adjust that number later, as we gain more experience with it.

@devanlooches
Copy link
Contributor Author

devanlooches commented Aug 6, 2022

Cool I will implement all of this shortly its a pretty quick fix.

Edit: Mostly :)

@hannobraun
Copy link
Owner

Nice, thank you!

@devanlooches
Copy link
Contributor Author

devanlooches commented Aug 6, 2022

Here is the fix. Unfortunately I'm having trouble putting the window on the bottom right by default (and still having it able to be dragged). If anyone could help me with that it would be great.

@devanlooches devanlooches requested a review from hannobraun August 6, 2022 16:07
@devanlooches
Copy link
Contributor Author

One other thing I was thinking is it might be a good idea to keep the 5 status messages unless there is a compiler error in which case it might be a good idea to clear them in order to save space for the compiler error message (And also not show it more than once).

@hannobraun
Copy link
Owner

Thank you, @devanlooches, I'll take a look later!

Copy link
Owner

@hannobraun hannobraun left a comment

Choose a reason for hiding this comment

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

Thanks for the latest changes, @devanlooches! And sorry for the delay; I was out and about for much of the weekend.

Unfortunately I'm having trouble putting the window on the bottom right by default (and still having it able to be dragged). If anyone could help me with that it would be great.

Unfortunately I don't know either (I don't have much experience with egui). We can always improve it later! (And maybe the lower-right corner isn't even the best location.)

One other thing I was thinking is it might be a good idea to keep the 5 status messages unless there is a compiler error in which case it might be a good idea to clear them in order to save space for the compiler error message (And also not show it more than once).

That sounds good!

I think it makes sense to distinguish between different kinds of status updates (info, error, etc.) and make decision based on what types we currently have.


I'm merging this now! There's still room for improvement, of course, but this is already much better than what we currently have. Thanks you again, @devanlooches!

Comment on lines +16 to +18
if self.status.len() >= 5 {
self.clear_status();
}
Copy link
Owner

Choose a reason for hiding this comment

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

I think this implementation is a bit weird, as it results in lots of statuses being removed at once. Might be better to just remove the oldest status, until only 5 are left. Maybe VecDeque is a better type for this than Vec, as it provides convenient push_back/pop_front methods.

But that's something that can be fixed later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem! Thanks for the feedback! Your right, VecDeque does look convenient. I'll look into it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants