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

use SelectableText in GUI for instance info #3852

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

levkropp
Copy link
Contributor

@levkropp levkropp commented Dec 18, 2024

closes #3821

This PR changes most Text widgets in the Instances and VM Details pages to SelectableText
SelectableText does not support overflow: ellipsis so I have set maxLines: 1 to ensure that table cells are still 1 line max.

I did not make the instance's name selectable in the Instances page because the name column has unique behavior implemented with VMNameLink where clicking it links to the instance's shell. Instead, the name of the instance on the Instance details/shell page is now selectable.

Screencast_20250115_131745.webm

@levkropp levkropp linked an issue Dec 18, 2024 that may be closed by this pull request
Copy link

codecov bot commented Dec 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.02%. Comparing base (eeed24d) to head (e5357f0).
Report is 30 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3852      +/-   ##
==========================================
- Coverage   89.02%   89.02%   -0.01%     
==========================================
  Files         255      255              
  Lines       14578    14577       -1     
==========================================
- Hits        12978    12977       -1     
  Misses       1600     1600              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@levkropp levkropp changed the title change Text to SelectableText in GUI for instance info use SelectableText in GUI for instance info Dec 18, 2024
@levkropp levkropp force-pushed the gui-make-text-selectable branch from 251cb44 to 1719d61 Compare December 18, 2024 23:16
src/client/gui/lib/extensions.dart Outdated Show resolved Hide resolved
@@ -107,3 +107,45 @@ extension NullableMap<T> on T? {
}
}
}

/// A custom wrapper for SelectableText with a white popup (right-click) menu.
class WhitePopupMenuSelectableText extends StatelessWidget {
Copy link
Contributor

@andrei-toterman andrei-toterman Dec 19, 2024

Choose a reason for hiding this comment

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

That's kind of a lengthy name IMO. You can name it just SelectableText and use import 'package:flutter/material.dart' hide SelectableText; so that you can use yours without interfering with Flutter's.
Also, you can move this to its own file. extensions.dart is more for defining useful extension methods on types rather than for widgets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can name it just SelectableText and use import 'package:flutter/material.dart' hide SelectableText; so that you can use yours without interfering with Flutter's.

When I tried to do this I wasn't able to define the contextMenuBuilder parameter: The contextMenuBuilder parameter is not defined in the SelectableText widget.
So instead I moved the class into its own file at selectable_text.dart and renamed it to WhiteSelectableText

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it was because your SelectableText was conflicting with Flutter's here. You can circumvent this by importing Flutter under an alias in just this file. Take a look here where there is a custom Tooltip widget that wraps Flutter's Tooltip.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand if this feels weird, and maybe it creates confusion. But IMO it's better than coming up with more elaborate names for widgets just because Flutter used them first. I'm open to criticism though!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Reusing standard names can be confusing. How about a shorter name for middle ground? Something like MySelectableText, CustomSelectableText, MPSelectableText? Or would PopupSelectableText be accurate enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reusing standard names can be confusing.

It seems like we already have precedent for overwriting standard widgets in the GUI (e.g. Tooltip in src/client/gui/lib/tooltip.dart and Switch in src/client/gui/lib/switch.dart), but we can think about refactoring to a unified naming convention for all of these instead of overwriting the widgets entirely. Thoughts on this @andrei-toterman ?

Copy link
Contributor

Choose a reason for hiding this comment

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

My vote is for MpSelectableText or for something like

import '../selectable_text.dart' as mp;

final selectableText = mp.SelectableText(...);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

something like

import '../selectable_text.dart' as mp;

final selectableText = mp.SelectableText(...);

I think this solution is the most elegant one because we only have to make changes to the imports and calls, and we don't have to change selectable_text.dart which keeps its implementation the same for other material.dart widget overrides we are doing (in switch.dart and tooltip.dart)

Whatever we choose, we also don't need to hide SelectableText when importing material.dart anymore. but maybe we should keep that for clarity?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, since we don't need to keep the hide, I don't thing we should. It's clear that the widgets are not the default Flutter ones.
I also think that introducing that namespace is the most elegant in terms of looks. I think it's nicer that having the prefix be part of the class name. But having the prefix in the class name like MpSelectableText has the advantage that it would be easier for the IDE to help you with importing it. Adding the as mp is not something an IDE would just know to do by default.
So yeah, I'm a bit torn between those two 😅
Of course, whatever solution we choose, we'll have to apply it to switch.dart and tooltip.dart as well.

@levkropp levkropp force-pushed the gui-make-text-selectable branch 3 times, most recently from 251195c to 4ba5a94 Compare December 19, 2024 20:14
@levkropp levkropp force-pushed the gui-make-text-selectable branch from 4ba5a94 to 0d04f37 Compare December 20, 2024 19:12
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.

Make text selectable
3 participants