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

WIP: rough cut of ScrollPanel --- LANTERNA-491 #492

Closed
wants to merge 76 commits into from

Conversation

ginkoblongata
Copy link
Contributor

@ginkoblongata ginkoblongata commented Jun 14, 2020

Creating this PR early before completion in case there is any comments which can gather.

@ginkoblongata ginkoblongata changed the title WIP: rough cut of ScrollPanel WIP: rough cut of ScrollPanel --- LANTERNA-491 Jun 14, 2020
@ginkoblongata
Copy link
Contributor Author

ginkoblongata commented Jun 18, 2020

testing with this:

$ mvn clean install ; java -cp "target/lanterna-3.1.0-SNAPSHOT-tests.jar:target/lanterna-3.1.0-SNAPSHOT.jar" -agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=5005 com.googlecode.lanterna.issue.Issue490 --mouse-move

*
* @author ginkoblongata
*/
public class TerminalRect {
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 TerminalRectangle would be a better name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will change it.

public final int x;
public final int y;
public final int width;
public final int height;
Copy link
Owner

Choose a reason for hiding this comment

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

It seems redundant to have both TerminalPosition and TerminalSize and also store x, y, width and height?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the intention is to make the usability of this component more fluid for the different ways in which it may be used.


@Override
public int getVerticalScrollPosition() {
return - scrollOffset.getRow();
Copy link
Owner

Choose a reason for hiding this comment

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

Is the - a typo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a typo, but redoing this part.
The ScrollableBox interface kinda became too cumbersome, so taking a draft 2 approach.

}

@Override
public void invalidate() {
Copy link
Owner

Choose a reason for hiding this comment

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

Don't think you need to override this, it should work anyway. The panel is marked as invalid in the Panel class, which forces a re-render of the whole thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, yep works without this method, using existing implementation.

scrollableBox.invalidate();
}

public class ScrollPanelLayoutManager extends BorderLayout {
Copy link
Owner

Choose a reason for hiding this comment

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

Does this need to be public? It looks like you wouldn't need to call this outside of ScrollPanel?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will make package protected.

* @return Itself
*/
public synchronized TextBox setCaretPosition(int line, int column) {
Copy link
Owner

Choose a reason for hiding this comment

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

This is a breaking change, please keep the original signature (you can forward the call into your new signature)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will clean this up once revisiting this PR.

@ginkoblongata
Copy link
Contributor Author

Just dropping a note here, haven't forgot about this, hopefully some more activity in a couple weeks.

@mabe02
Copy link
Owner

mabe02 commented Sep 27, 2020

@ginkoblongata would you be opening a new PR or update this one?

@ginkoblongata
Copy link
Contributor Author

I would like to keep this one open, and eventually have it accepted for merge. But I will make another 1 or 2 slices off I think with new PR.

@ginkoblongata
Copy link
Contributor Author

Just dropping another note, I am still committed to this change. Other commitments are currently taking priority at this time still.

@scolastico
Copy link
Contributor

Would be very interrested to see that feature.

@mabe02
Copy link
Owner

mabe02 commented Jul 27, 2023

Stale

@mabe02 mabe02 closed this Jul 27, 2023
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.

None yet

5 participants