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

Sort by usage #1212

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

lucile98-ode
Copy link

Modification of the database to add a column which corresponds to the number of uses.
Added a method to update the value on each use.
To enable sorting by this value :)

Copy link
Member

@TheLastProject TheLastProject left a comment

Choose a reason for hiding this comment

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

Haven't tested it yet, but noticing multiple issues already.

@@ -47,6 +47,7 @@ public static class LoyaltyCardDbIds {
public static final String LAST_USED = "lastused";
public static final String ZOOM_LEVEL = "zoomlevel";
public static final String ARCHIVE_STATUS = "archive";
public static final String USAGE_NUMBER = "usagenumber";
Copy link
Member

Choose a reason for hiding this comment

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

I think I'd much prefer this to be named USAGE_COUNT and usagecount. To me that is more logical.

Copy link
Author

Choose a reason for hiding this comment

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

Done :)

Comment on lines +70 to +71

UsageNumber
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
UsageNumber
UsageCount

Copy link
Author

Choose a reason for hiding this comment

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

Done !

@@ -189,6 +193,7 @@ public void onUpgrade(SQLiteDatabase db, int oldVersion, int newVersion) {
LoyaltyCardDbIds.HEADER_COLOR + " INTEGER," +
LoyaltyCardDbIds.CARD_ID + " TEXT not null," +
LoyaltyCardDbIds.BARCODE_ID + " TEXT," +
LoyaltyCardDbIds.USAGE_NUMBER + " INTEGER DEFAULT '0', " +
Copy link
Member

Choose a reason for hiding this comment

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

This is wrong, this will only be executed when users are on database version 9. So existing installations won't get the usage column. This should be removed (there should be an extra upgrade at the end and the DATABASE_VERSION variable needs to be increased).

Copy link
Author

Choose a reason for hiding this comment

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

in progress..

@@ -202,6 +207,7 @@ public void onUpgrade(SQLiteDatabase db, int oldVersion, int newVersion) {
LoyaltyCardDbIds.HEADER_COLOR + " ," +
LoyaltyCardDbIds.CARD_ID + " ," +
LoyaltyCardDbIds.BARCODE_ID + " ," +
LoyaltyCardDbIds.USAGE_NUMBER + " , " +
Copy link
Member

Choose a reason for hiding this comment

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

See above.

Copy link
Author

Choose a reason for hiding this comment

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

Done

@@ -214,6 +220,7 @@ public void onUpgrade(SQLiteDatabase db, int oldVersion, int newVersion) {
LoyaltyCardDbIds.HEADER_COLOR + " ," +
LoyaltyCardDbIds.CARD_ID + " ," +
LoyaltyCardDbIds.BARCODE_ID + " ," +
LoyaltyCardDbIds.USAGE_NUMBER + " , " +
Copy link
Member

Choose a reason for hiding this comment

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

See above.

Copy link
Author

Choose a reason for hiding this comment

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

Done

Comment on lines +432 to +433
DBHelper.updateLoyaltyCardUsageNumber(database, loyaltyCardId);

Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to put this either near the top or all the way at the bottom for clarity.

Copy link
Author

Choose a reason for hiding this comment

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

Hi, i don't know where to put this function, i think it going to be null :/

Comment on lines +474 to +475
//this.database

Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary debug comment.

Suggested change
//this.database

@@ -308,6 +308,7 @@ private void setCenterGuideline(int zoomLevel) {

@Override
protected void onCreate(Bundle savedInstanceState) {

Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary whitespace change.

Suggested change

@@ -466,7 +469,10 @@ public void onLayoutChange(View v, int left, int top, int right, int bottom, int
iconImage.setClipBounds(new Rect(left, top, right, bottom));
}
});

Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary whitespace change.

Suggested change

@@ -67,7 +67,7 @@ public class MainActivity extends CatimaAppCompatActivity implements LoyaltyCard
private int mLoyaltyCardCount = 0;
protected String mFilter = "";
protected Object mGroup = null;
protected DBHelper.LoyaltyCardOrder mOrder = DBHelper.LoyaltyCardOrder.Alpha;
protected DBHelper.LoyaltyCardOrder mOrder = DBHelper.LoyaltyCardOrder.UsageNumber;
Copy link
Member

Choose a reason for hiding this comment

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

Please don't change the default sorting style.

Copy link
Author

Choose a reason for hiding this comment

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

Done

@TheLastProject TheLastProject linked an issue Feb 5, 2023 that may be closed by this pull request
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.

Order by most commonly used
2 participants