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

[Feature Request] Ability to provide a label text builder #2

Closed
vishnukvmd opened this issue Feb 8, 2021 · 20 comments · Fixed by #3
Closed

[Feature Request] Ability to provide a label text builder #2

vishnukvmd opened this issue Feb 8, 2021 · 20 comments · Fixed by #3

Comments

@vishnukvmd
Copy link
Contributor

Firstly, thanks for the great library, it has been a blessing!

Now, while using the DraggableScrollbar, I'd like to provide a label text next to the "thumb".

The original library exposes a labelTextBuilder which does this. Would it be possible for you to expose something similar?

Thanks again!

@deakjahn
Copy link
Owner

deakjahn commented Feb 8, 2021

Could this be something that was added after I copied that code?

@vishnukvmd
Copy link
Contributor Author

I don’t think so, their codebase has not undergone any changes in over a year.

@deakjahn
Copy link
Owner

deakjahn commented Feb 8, 2021

Passing the builder is relatively straightforward but what shall we do with the label text in the end?

@vishnukvmd
Copy link
Contributor Author

Pass it on to the DraggableScrollbar and let it render it?

@deakjahn
Copy link
Owner

deakjahn commented Feb 8, 2021

Yep, but the question is how to render it? Now I can see, this is something I was probably glad to remove in the first place. :-)

@vishnukvmd
Copy link
Contributor Author

I haven't explored the code completely, but it should be possible to render a label next to the thumb in lib/src/draggable_scrollbar.dart?

@deakjahn
Copy link
Owner

deakjahn commented Feb 8, 2021

You probably also need the labelConstraints and the full ScrollLabel from that code.

@deakjahn
Copy link
Owner

deakjahn commented Feb 8, 2021

I have a feeling now that it would be much cleaner to just build extra thumbs that contain a text as well as a shape, rather than using the existing thumbs and injecting an extra text beside them...

I mean, instead of using DraggableScrollbarThumbs.SemicircleThumb, for instance, we just copy that code, and make it a Row that has both your label and the original thumb.

@vishnukvmd
Copy link
Contributor Author

That sounds great. If you could pass the firstShown index here, that would be perfect.

@deakjahn
Copy link
Owner

deakjahn commented Feb 8, 2021

I uploaded something, there could be something like this:

static Widget SemicircleThumbLabel(Color backgroundColor, Color drawColor, double height, int index) {
  return Row(
    children: [
      Text(index.toString()),
      SemicircleThumb(backgroundColor, drawColor, height, index),
    ],
  );
}

Of course, this is ugly, puts the whole package to the left (axis alignments are lacking) and the label is nothing to write home about, just food for thought. If you come up with a nice one, we can put it into there.

@deakjahn deakjahn reopened this Feb 8, 2021
@vishnukvmd
Copy link
Contributor Author

This works quite well! I will think about ways to generalize it, although I'm happy with how extensible the thumb-builder is right now.

Follow up question, is there any reason why you haven't reused the flutter-draggable-scrollbar out of the box? I'm curious because there are certain niceties there like the scroll thumb animations (fade-in and fade-out), that are missing here.

@deakjahn
Copy link
Owner

deakjahn commented Feb 9, 2021

Yes, very probably there was, just that I can't recall it any more after some months. :-) I had to modify it too heavily, probably to introduce some extra communication that binds the list and the scrollbar together. The base problem is that the ScrollablePositionedList used inside doesn't work like a regular listview when scrollbars are concerned. As far as I can remember, I faced the problem of modifying one to work with the other. And modifying the list itself would have been much more complicated, if not impossible, so I opted for modifying the simpler component. I couldn't use both as is, I think.

@vishnukvmd
Copy link
Contributor Author

Hey @deakjahn, thanks again for the changes. But the index attribute that is passed within the thumbBuilder is stale a lot of times.

I was wondering if the currentFirstIndex within DraggableScrollbar should be updated when the ItemPositionsListener within huge_listview.dart is fired.

@deakjahn
Copy link
Owner

deakjahn commented Feb 14, 2021

In theory, it looks freshly obtained on every frame. I never used your approach to add a label, I actually have a large first letter display in the background of my dictionary display but I didn't notice any problems with that.

When do you experience slateness? On manual scrolling or during some other operations? firstShown also receives the very same value now. What's more, even the position of the thumb itself relies on this value, so if it gets stale, that should be visible in the movement of the thumb, too.

@deakjahn deakjahn reopened this Feb 14, 2021
@vishnukvmd
Copy link
Contributor Author

The issue happens on scrolling up and down with either the dragbar and or the scroll-view. If you're curious, you could check out the app here: https://github.com/ente-io/frame/tree/smooth_scroll

From the debugger, it seems that _sendFirst is called correctly every time. Perhaps in this function we should trigger a notification to DraggableScrollbar?

@deakjahn
Copy link
Owner

But: _sendScroll() is called by the listener itself and that's where firstShown is actually triggered from.

@vishnukvmd
Copy link
Contributor Author

Yes, firstShown does give the correct value, but the thumbBuilder does not.

@deakjahn
Copy link
Owner

Yep, I understand, still firstShown is called from this listener handler and uses the very same function to get the value, so, right now, I still don't understand why...

@vishnukvmd
Copy link
Contributor Author

Hey, I’m stuck on some other tasks. I’ll reopen this issue after digging in more. Thanks!

@vishnukvmd
Copy link
Contributor Author

Sorry for the delay @deakjahn. #3 fixes the issue I pointed out.

Thanks again for open-sourcing this!

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 a pull request may close this issue.

2 participants