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.

Sign up for GitHub

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

Jump to bottom

Auto Sort labels bug and other label handling issues #5630

Closed
wants to merge 1 commit into from
Closed

Auto Sort labels bug and other label handling issues #5630

wants to merge 1 commit into from

Conversation

boutinb
Copy link
Contributor

@boutinb boutinb commented Jul 18, 2024

This solves several issues:

  • The "Auto Sort Label per value" setting (true per default in the preferences) is not saved in the database. Now this setting is saved in the database and thus also in the JASP file.
  • The "Auto Sort Label per value" works also for non numeric values. Now the labels are per default also alphabetically ordered.
  • The Variable Window does not show always the selected column, especially when navigating in the Data view with the keyboard: the Variable Window was showing still the old selected column.
  • The up/down/reverse buttons were not visible or/and enabled in the right way. Now if Auto Sort is on, all the other buttons are removed, except the 'Reverse order of all numerical values' if at least 2 labels have numerical values. If Auto Sort is off, all other buttons are displayed, and are enabled only if they really can change the order (the up button is disabled for example if the the selected row is the first one).

@boutinb boutinb requested a review from RensDofferhoff July 18, 2024 11:26
@boutinb boutinb changed the title Auto Sort labels does bug and other label handling issues Auto Sort labels bug and other label handling issues Sep 6, 2024
@boutinb boutinb requested a review from JorisGoosen September 6, 2024 08:08
@boutinb
Copy link
Contributor Author

boutinb commented Sep 12, 2024

This is now ready for review.

@JorisGoosen
Copy link
Contributor

Ok Ive added a fix already for the autosort not being stored: 0054568

JorisGoosen
JorisGoosen reviewed Sep 17, 2024
View reviewed changes
Copy link
Contributor

@JorisGoosen JorisGoosen left a comment

Choose a reason for hiding this comment

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

I have some questions on the code and have the feeling you are changing quite a few things for little gain.

Maybe we can have a look together next week when you are back?

CommonData/column.h

public:
static Column * addColumn(DataSet* data, int index = -1, const std::string & name = "", columnType colType = columnType::scale, computedColumnType computedType = computedColumnType::notComputed, bool alterDataSetTable = true);
static Column * loadColumn(DataSet* data, int index);
Copy link
Contributor

Choose a reason for hiding this comment

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

This function obviously should be on DataSet...

CommonData/column.cpp
for(Label * label : _labels)
doublevec asc = valuesNumericOrdered();

if (asc.empty())
Copy link
Contributor

Choose a reason for hiding this comment

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

So with this change it either orders the numeric ones or the non-numerics if there are no numerics?

Why not order both?

@JorisGoosen
Copy link
Contributor

lets talk about this tomorrow

JorisGoosen
JorisGoosen reviewed Oct 15, 2024
View reviewed changes
Desktop/components/JASP/Widgets/LabelEditorWindow.qml
visible: !columnModel.autoSort || columnModel.rowsTotal - columnModel.firstNonNumericRow > 1 //If there are at least 2 non numerics there is something to reverse

visible: !columnModel.autoSort
enabled: columnModel.rowCount() > 1
Copy link
Contributor

Choose a reason for hiding this comment

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

dont use rowCount() in a binding because it wont update on changes...

@JorisGoosen JorisGoosen mentioned this pull request Oct 15, 2024
Merged
@JorisGoosen
Copy link
Contributor

All those changes to column, dataset and databaseinterface seem to be useless and change the logic in a bunch of places.
I dont think this is necessary and another way is here: #5700

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JorisGoosen JorisGoosen JorisGoosen left review comments

@RensDofferhoff RensDofferhoff Awaiting requested review from RensDofferhoff

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@boutinb @JorisGoosen