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

Proposal: add SelectionStart and SelectionEnd to TrackBar #2642

Open
hughbe opened this issue Jan 6, 2020 · 10 comments
Open

Proposal: add SelectionStart and SelectionEnd to TrackBar #2642

hughbe opened this issue Jan 6, 2020 · 10 comments
Assignees
Labels
api-approved (4) API was approved in API review, it can be implemented design-discussion Ongoing discussion about design without consensus help wanted Good issue for external contributors tenet-compatibility-OS Compatibility with OS features
Milestone

Comments

@hughbe
Copy link
Contributor

hughbe commented Jan 6, 2020

You can set the TBS_ENABLESELRANGE on TrackBar and send the TBM_SETSELSTART and TBM_SETSELSTART to set the selection range
See docs

Example:
Screenshot 2020-01-06 at 16 31 38
Screen Recording 2020-01-06 at 04 32 pm

We could add the following APIs to TrackBar:

public class TrackBar
{
    ...
    public bool ShowSelectionRange { get; set; }
    public int SelectionStart { get; set; }
    public int SelectionEnd { get; set; }
    ...
}

Discussion

  • What should the default values of SelectionStart and SelectionEnd be if we haven't enabled selections? In my experiments I set them to 0 which worked nicely.
  • In my prototype implementation, I set ShowSelectionRange to true if the user sets SelectionStart/SelectionEnd. One option could be to remove ShowSelectionRange and set SelectionStart/SelectionEnd to minus one or have some logic that sets the TrackBar style to TBS_ENABLESELRANGE if it is set to a custom value. However, this is negative as it leaves us with no way of removing the selection indicators once they've been created.
  • Do we want SelectionStart and SelectionEnd or SelectionStart and SelectionLength or some sort of SelectionRange struct that encapsulates this? (e.g MonthCalendar.SelectionRange)
@RussKie RussKie added api-suggestion (1) Early API idea and discussion, it is NOT ready for implementation design-discussion Ongoing discussion about design without consensus labels Jan 7, 2020
@weltkante
Copy link
Contributor

weltkante commented Jan 7, 2020

Chosing -1 as sentinel value is bad because I think you can have a negative Minimum. Letting users chose a value centered around zero (e.g. between -100 and +100) is probably a common usecase of this control.

Having a separate boolean property is how this is usually done in WinForms so that would be my first choice here as well.

One question I'm not clear on is whether this is an input property (can the user edit the selection) or is it informative only (e.g. for presenting the current range of values within a multi-selection). If the user cannot edit this through user input I'm hesitant to name it "Selection" even if the native API names it this way.

@hughbe
Copy link
Contributor Author

hughbe commented Jan 7, 2020

One question I'm not clear on is whether this is an input property (can the user edit the selection) or is it informative only (e.g. for presenting the current range of values within a multi-selection). If the user cannot edit this through user input I'm hesitant to name it "Selection" even if the native API names it this way.

From what I can see it is informative only. In my example gif I showed how it can be connected to the value of the thumb as an example use case. Open to any other suggestions for names (perhaps SelectionIndicatorStart?)

@weltkante
Copy link
Contributor

weltkante commented Jan 7, 2020

I'm not too picky about naming, its just that Selection tends to be connected with user input. If there's desire to keep the name its ok as well, but I want it be discussed. At this point its probably best to throw a bunch of suggestions in, I'd be ok with SelectionIndicatorStart. Some alternatives could be dropping the Selection prefix (just IndicatorStart) or MarkerStart. "Indicator" and "Marker" are common terms in charting when annotating an axis and vaguely resemble the functionality of what we have here.

Do we want SelectionStart and SelectionEnd or SelectionStart and SelectionLength or some sort of SelectionRange struct that encapsulates this?

After thinking about this I tend to go for Start/End because this maps most closely to what the control is doing: the user is selecting a value and you put markers at two other values to mark a range. This allows easy binding of the properties against two values, going for Start/Length would more probably require coding for calculating the length.

Chosing a struct would probably not be very compatible with the WinForms binding system, but I can't tell for sure since there is little precedence. Maybe it would just work out firing INotifyPropertyChanged event when either property changes on the struct and the Binding engine will resolve the member access? Might worry about boxing though, I think binding works on objects. Strike that, structs are incompatible with the binding engine. Since this is not a user input you always will want to write it through the binding engine. You can't write struct members through the binding engine since they'll be a boxed copy and not affect the struct owned by the control.

@RussKie RussKie added the tenet-compatibility-OS Compatibility with OS features label Jan 8, 2020
@RussKie
Copy link
Member

RussKie commented Jan 8, 2020

@hughbe could you please share your test app (e.g. publish it to GitHub under your account)? It would be great to have some hands on experience to make better informed decisions.

I'm leaning towards defining a new class similar to SelectionRange. This way we can provide a better the Designer support. And it would be consistent with the existing API.
It is unfortunate SelectionRange name is already taken. Though we can probably do a nested class within the TrackBar class.
Another control I can think of that exposes a selection configuration is LinkLabel.LinkArea. But it is not the same functionality as TrackBar though.

  • What should the default values of SelectionStart and SelectionEnd be if we haven't enabled selections? In my experiments I set them to 0 which worked nicely.

These values must also be within the range.

  • In my prototype implementation, I set ShowSelectionRange to true if the user sets SelectionStart/SelectionEnd.

What do you think of ShowSelection? This would compliment ShowThumb too...

@RussKie
Copy link
Member

RussKie commented Jan 22, 2020

The team had some preliminary discussions about enhancing the control.
Could you spare few minutes and provide answers to the following questions so we can take it to the next level:

  • Will VS Designer need to support the feature? If yes, describe how you expect it to funсtion.
  • What impact will it have on accessibility?
  • Will this feature need to be localized or be localizable?

With the regards to this specific feature the first question is where I expect most questions.

@RussKie RussKie added the 📭 waiting-author-feedback The team requires more information from the author label Jan 22, 2020
@merriemcgaw merriemcgaw added this to the 5.0 milestone Jan 23, 2020
@RussKie RussKie added the api-ready-for-review (2) API is ready for formal API review; applied by the issue owner label Jan 23, 2020
@terrajobst terrajobst added api-approved (4) API was approved in API review, it can be implemented and removed api-ready-for-review (2) API is ready for formal API review; applied by the issue owner labels Mar 5, 2020
@terrajobst
Copy link
Member

terrajobst commented Mar 5, 2020

Video

  • We should probably follow TextBoxBase and use SelectionStart and SelectionLength, because that avoids ordering issues and the need for sentinel values. @RussKie, it would be good to check what the throw behavior is for TextBoxBase.
  • We considered making it method like SetSelection(int start, int length) but that would mean you can't set the selection using the designer, which feels odd
namespace System.Windows.Forms
{
    public partial class TrackBar
    {
        public bool ShowSelectionRange { get; set; }
        public int SelectionStart { get; set; }
        public int SelectionLength { get; set; }
    }
}

@hughbe What the other properties you show in your UI? Like ShowThumb and ThumbWidth?

@RussKie
Copy link
Member

RussKie commented Mar 5, 2020

Few questions:

  • Throw if SelectionLength < 0
  • Clamp SelectionStart to either Minimum or Maximum if outside the boundaries
  • Clamp if SelectionStart + SelectionLength > Maximum to Maximum

@ghost

This comment has been minimized.

@weltkante

This comment has been minimized.

@ghost ghost removed the 💤 no-recent-activity label Mar 24, 2020
@dotnet dotnet deleted a comment Apr 8, 2020
@ghost ghost removed the 💤 no-recent-activity label Apr 8, 2020
@RussKie RussKie modified the milestones: 5.0 Previews 1-4, 5.0 Apr 20, 2020
@RussKie RussKie added help wanted Good issue for external contributors and removed 📭 waiting-author-feedback The team requires more information from the author labels Apr 23, 2020
@merriemcgaw merriemcgaw modified the milestones: 5.0, Future Sep 1, 2020
@terrajobst terrajobst removed the api-suggestion (1) Early API idea and discussion, it is NOT ready for implementation label Jun 25, 2021
@RussKie RussKie removed this from the Future milestone Aug 27, 2021
@RussKie RussKie modified the milestones: 7.0, Future Aug 27, 2021
@elachlan
Copy link
Contributor

elachlan commented Jan 9, 2023

@conorgee This is another trackbar improvement if you want to give it a go?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved (4) API was approved in API review, it can be implemented design-discussion Ongoing discussion about design without consensus help wanted Good issue for external contributors tenet-compatibility-OS Compatibility with OS features
Projects
None yet
Development

No branches or pull requests

8 participants