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

TaskProgressView - new flag to retain tasks when succeeded, cancelled or failed #1127

Merged
merged 5 commits into from Jun 6, 2019

Conversation

Projects
None yet
3 participants
@nicolabeghin
Copy link
Contributor

commented Mar 29, 2019

As discussed in #1118, a simple way to have TaskProgressView retaining a task after it's succeeded, cancelled or failed

@nicolabeghin

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2019

Any news on this PR? I didn't see a specific branch for 11.x, hope it will be merged in there too. Thanks!

@nicolabeghin

This comment has been minimized.

Copy link
Contributor Author

commented May 19, 2019

Up! Thanks 😁

@abhinayagarwal

This comment has been minimized.

Copy link
Collaborator

commented Jun 1, 2019

@dlemmermann Can you review this PR?

@dlemmermann

This comment has been minimized.

Copy link
Contributor

commented Jun 1, 2019

@@ -62,20 +62,24 @@
*/
public class TaskProgressView<T extends Task<?>> extends ControlsFXControl {

private boolean retainTaskMode = false;

This comment has been minimized.

Copy link
@dlemmermann

dlemmermann Jun 2, 2019

Contributor

On controls we use properties, not fields. Please use BooleanProperty retainTasks = new SimpleBooleanProperty(this, "retainTasks", false); Also no reason to call it a "mode". A simple "retainTasks" is all we need.

This comment has been minimized.

Copy link
@dlemmermann

dlemmermann Jun 2, 2019

Contributor

Also, now that the tasks are not removed automatically upon completion it would be good to have a "clear" button to manually remove them. I just noticed that the skin of TaskProgressView already creates a BorderPane but does not use it. In the end the skin adds the ListView directly to the control, the BorderPane does not get used. I would suggest you set the ListView in the center of the BorderPane and place a button at the bottom of the list view. Only show the button if the retainTasks property is "true". This can be done by binding the "visible" and the "managed" property of the clear button to the "retainTasks" property. This also shows you how important it is to use a property for "retainTasks" instead of just a field.

This comment has been minimized.

Copy link
@dlemmermann

dlemmermann Jun 2, 2019

Contributor

I checked and I think the CSS styling will still work even when adding the BorderPane.

This comment has been minimized.

Copy link
@nicolabeghin

nicolabeghin Jun 2, 2019

Author Contributor

hi @dlemmermann just committed the refactored code to use properties in 9cfbe5e. Honestly I don't think I have the capability to "properly" add the additional button, so if ok for you I'll keep it down to this!

@@ -71,7 +73,7 @@ public TaskProgressView() {
getStyleClass().add("task-progress-view");

EventHandler<WorkerStateEvent> taskHandler = evt -> {
if (!retainTaskMode) {
if (!retainTasks.get()) {

This comment has been minimized.

Copy link
@dlemmermann

dlemmermann Jun 3, 2019

Contributor

if (!isRetainTasks()) would read better.

This comment has been minimized.

Copy link
@nicolabeghin

nicolabeghin Jun 3, 2019

Author Contributor

done!

*/
public void setRetainTaskMode(boolean retainTaskMode) {
this.retainTaskMode = retainTaskMode;
public BooleanProperty retainTasksProperty() {

This comment has been minimized.

Copy link
@dlemmermann

dlemmermann Jun 3, 2019

Contributor

please make method final

This comment has been minimized.

Copy link
@nicolabeghin

nicolabeghin Jun 3, 2019

Author Contributor

done!

*/
public boolean isRetainTaskMode() {
return retainTaskMode;
public boolean isRetainTasks() {

This comment has been minimized.

Copy link
@dlemmermann

dlemmermann Jun 3, 2019

Contributor

please make method final

This comment has been minimized.

Copy link
@nicolabeghin

nicolabeghin Jun 3, 2019

Author Contributor

done!

@@ -62,7 +64,7 @@
*/
public class TaskProgressView<T extends Task<?>> extends ControlsFXControl {

private boolean retainTaskMode = false;
private BooleanProperty retainTasks = new SimpleBooleanProperty(this, "retainTasks", false);

This comment has been minimized.

Copy link
@dlemmermann

dlemmermann Jun 3, 2019

Contributor

please make field final.

This comment has been minimized.

Copy link
@nicolabeghin

nicolabeghin Jun 3, 2019

Author Contributor

done!

@@ -62,7 +64,7 @@
*/
public class TaskProgressView<T extends Task<?>> extends ControlsFXControl {

private boolean retainTaskMode = false;
private BooleanProperty retainTasks = new SimpleBooleanProperty(this, "retainTasks", false);

This comment has been minimized.

Copy link
@dlemmermann

dlemmermann Jun 3, 2019

Contributor

You also forgot the property accessor method "retainTasksProperty()" (also final). And in JavaFX controls there is this convention to keep the field and its three methods (xyzProperty(), isXyzProperty(), setXyzProperty()) grouped together in the source code.

This comment has been minimized.

Copy link
@nicolabeghin

nicolabeghin Jun 3, 2019

Author Contributor
  • retainTasksProperty() seems there already
  • setXyzProperty() doesn't make sense since the property has been made final right?
@nicolabeghin

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

Committed @dlemmermann requests in 4cbadb5!

@dlemmermann

This comment has been minimized.

Copy link
Contributor

commented on 4cbadb5 Jun 3, 2019

There is now no method to set the property value!! You forgot setRetainTasks(boolean).

This comment has been minimized.

Copy link
Contributor Author

replied Jun 3, 2019

sorry my bad :D committed ca37daf

@dlemmermann
Copy link
Contributor

left a comment

Please also finish the java docs. Some "@return" are blank. Also add a period / full stop at the end of the method documentation.

@dlemmermann
Copy link
Contributor

left a comment

You also need to change the end year in the license header to 2019. Whenever we touch / modify a file this needs to get updated.

@nicolabeghin

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

committed in ee04936

@abhinayagarwal abhinayagarwal merged commit 821bbad into controlsfx:master Jun 6, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.