Skip to content
This repository has been archived by the owner on Jan 10, 2024. It is now read-only.

Study Cards #495

Merged
merged 8 commits into from
Feb 21, 2018
Merged

Study Cards #495

merged 8 commits into from
Feb 21, 2018

Conversation

sebmarste
Copy link
Contributor

@sebmarste sebmarste commented Jul 18, 2017

first implementation of the study card feature (demo)
Issue #457

- card list
- adding cards
app/build.gradle Outdated
@@ -42,6 +43,9 @@ android {
exclude 'META-INF/maven/com.google.guava/guava/pom.xml'
}

dataBinding {
enabled = true
Copy link
Member

Choose a reason for hiding this comment

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

I didn't know about this stuff - cool! 👍

import de.tum.in.tumcampusapp.models.tumcabe.ChatMember;
import de.tum.in.tumcampusapp.models.tumcabe.ChatVerification;

public class CardsDetailActivity extends AppCompatActivity {
Copy link
Member

Choose a reason for hiding this comment

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

Please extend our Generic Classes - do not directly use the AppCompat stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

}

private boolean save() {
if (card.is_valid()) {
Copy link
Member

Choose a reason for hiding this comment

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

Invert these ifs please. e.g.:
if (!card.is_valid()) { return false; }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -38,6 +38,8 @@ public ActivityForLoadingInBackground(int layoutId) {
super(layoutId);
}

public ActivityForLoadingInBackground() { super(); }
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

refactored all this (using overwriting the setUpLayout method)

@@ -50,12 +50,21 @@ public BaseActivity(int layoutId) {
mLayoutId = layoutId;
}

public BaseActivity() { mLayoutId = null;}
Copy link
Member

Choose a reason for hiding this comment

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

ehhhm ??? Please explain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

@@ -51,10 +51,18 @@ public ProgressActivity(int layoutId) {
super(layoutId);
}

public ProgressActivity() { super(); }
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

@Override
protected void onCreate(Bundle savedInstanceState) {
super.onCreate(savedInstanceState);

if (this.mLayoutId != null) {
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be allowed to be null in the first place - thats why we don't have any default constructors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed (this was to inflate the layout with data binding)

public class StudyCard extends BaseObservable {

private int id;
//private ChatMember member;
Copy link
Member

Choose a reason for hiding this comment

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

Commented code should not be in git.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -127,6 +127,7 @@ static void updateAppWidgets(Context context, AppWidgetManager appWidgetManager,
static void updateAppWidget(Context context, AppWidgetManager appWidgetManager,
int appWidgetId, boolean forceLoadData) {
// Get the settings for this widget from the database
if (transportManager == null) transportManager = new TransportManager(context);
Copy link
Member

Choose a reason for hiding this comment

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

Bug fix? Please put this in a separate PR.

build.gradle Outdated
@@ -18,10 +18,11 @@ allprojects {
options.fork = true // Fork your compilation into a child process
options.forkOptions.setMemoryMaximumSize("4g") // Set maximum memory to 4g
options.compilerArgs << "-Xlint:all"
options.compilerArgs << "-Werror"
// options.compilerArgs << "-Werror"
Copy link
Member

Choose a reason for hiding this comment

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

Never touch this file. @pfent will be very angry...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the problem here is, that i get warnings (i assume caused by the data binding library) and with this flag set it will not compile with warnings... ill investigate this after the presentation

Copy link
Member

Choose a reason for hiding this comment

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

@pfent is there not some config file to add exceptions for those libs? I remember seeing it somewhere but not exactly...

Copy link
Contributor

Choose a reason for hiding this comment

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

Usually, you can always @SuppressWarnings of the specific thing that errors.
Please don't disable the -Werror, it really helps to keep developers to think about their code ❤️

Copy link
Contributor Author

@sebmarste sebmarste Jul 20, 2017

Choose a reason for hiding this comment

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

@pfend, @kordianbruck: These are the actual warnings I get:

Warning:No processor claimed any of these annotations: java.lang.SafeVarargs,android.databinding.BindingBuildInfo,org.simpleframework.xml.ElementList,android.support.annotation.NonNull,com.google.gson.annotations.SerializedName,org.simpleframework.xml.Root,retrofit2.http.PUT,android.annotation.SuppressLint,retrofit2.http.DELETE,retrofit2.http.GET,retrofit2.http.Path,org.simpleframework.xml.Text,android.support.annotation.Nullable,retrofit2.http.POST,retrofit2.http.Body,org.simpleframework.xml.Element,android.annotation.TargetApi,android.databinding.Bindable

/Users/sebastianstein/Google Drive/Uni/android praktikum/TumCampusApp/app/build/generated/source/apt/debug/de/tum/in/tumcampusapp/databinding/ActivityCardsQuizBinding.java
Warning:(59, 37) [cast] redundant cast to String
Warning:(85, 36) [cast] redundant cast to String
Warning:(180, 112) [unchecked] unchecked cast
required: ObservableField<Boolean>
found:    Object

/Users/sebastianstein/Google Drive/Uni/android praktikum/TumCampusApp/app/build/generated/source/apt/debug/de/tum/in/tumcampusapp/databinding/ActivityCardsDetailBinding.java
Warning:(52, 32) [cast] redundant cast to String
Warning:(78, 37) [cast] redundant cast to String
Warning:(104, 36) [cast] redundant cast to String

The first one appears since i enabled databinding for the project.
The others are cast warnings in the Binding Classes which are generated by the databinding library.
So I cant simply fix the cast problems there or add the @SuppressWarnings annotation.

But I totally understand why this flag is set and i am going to investigate on this after the "hot time" of the praktikum.

Copy link
Member

Choose a reason for hiding this comment

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

@kordianbruck
Copy link
Member

I think we should merge this for now and finish work on this in the coming semester.

…57-study-cards

# Conflicts:
#	app/build.gradle
#	app/src/main/java/de/tum/in/tumcampusapp/api/TUMCabeClient.java
#	app/src/main/java/de/tum/in/tumcampusapp/auxiliary/DrawerMenuHelper.java
#	app/src/main/res/values-de/strings.xml
#	app/src/main/res/values/strings.xml
#	app/src/main/res/xml/settings.xml
#	build.gradle
@kordianbruck kordianbruck merged commit 3fa455c into master Feb 21, 2018
@kordianbruck kordianbruck deleted the sebmarste/457-study-cards branch February 21, 2018 13:37
app/build.gradle Outdated
@@ -107,7 +107,7 @@ dependencies {
annotationProcessor "android.arch.lifecycle:compiler:$roomVersion"
annotationProcessor "android.arch.persistence.room:compiler:$roomVersion"
implementation "com.google.android.gms:play-services-location:$firebaseVersion"
implementation 'com.android.support.constraint:constraint-layout:1.0.2'
implementation 'com.android.support.constraint:constraint-layout:1.0.2'openinghours
Copy link
Contributor

Choose a reason for hiding this comment

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

no, please don't

Copy link
Member

Choose a reason for hiding this comment

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

upps

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants