feature/oauthDance -> dev

This change is

Reviewed 11 of 26 files at r2, 1 of 1 files at r3, 34 of 51 files at r4, 17 of 18 files at r5.
Review status: all files reviewed at latest revision, 45 unresolved discussions.

app/src/main/AndroidManifest.xml, line 90 at r5 (raw file):

			android:launchMode="singleTop"
			android:parentActivityName=".mainScreen.AddFeaturesActivity" />
		<activity

Remove (in the future, make sure to always use git add -p and only ever add the files that are "dev" ready)

app/src/main/java/edu/byu/suite/features/addDrop/controller/mySchedule/EnrolledFragment.java, line 11 at r5 (raw file):

import edu.byu.suite.features.addDrop.model.AddDropScheduledClassesWrapper;
import edu.byu.suite.features.addDrop.service.StudentsClient;
import edu.byu.support.activity.ByuActivity;

Remove. Also, remove from all other files. I'm assuming this got added when you used the cast. Now that we got rid of that, we should also be able to get rid of this

app/src/main/java/edu/byu/suite/features/addDrop/service/ClassesClient.java, line 37 at r5 (raw file):

	@Override
	public ByuApiGenerator.AuthType getAuthType() {

Whoa... What did we do here? Did you make BEARER_TOKEN the default or something?

app/src/main/java/edu/byu/suite/features/myFinancialCenter/controller/UnpaidChargesFragment.java, line 85 at r5 (raw file):

			public void onFailure(Call<List<Charge>> call, ByuError byuError) {
				dismissProgressDialog();
				if (byuError.getResponse() != null && byuError.getResponse().code() == 418) {

Was this fixing a bug?

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.java, line 122 at r5 (raw file):

			cancelAllCalls();
			swipeRefreshLayout.setRefreshing(true);
			enqueueCall(testingCenterApi.getScores(yearTerm), new ByuCallback<ScoresResponseWrapper>(this) {

Umm... Why did this get inlined?

app/src/main/java/edu/byu/suite/mainScreen/FavoritesActivity.java, line 215 at r5 (raw file):

				logout();
				break;
			case R.id.test_oauth:

Remove

app/src/main/java/edu/byu/suite/mainScreen/OAuthTokenTestActivity.java, line 1 at r5 (raw file):

package edu.byu.suite.mainScreen;

Remove

app/src/main/java/edu/byu/suite/mainScreen/SplashScreen.java, line 14 at r5 (raw file):

public class SplashScreen extends ByuActivity {

	// This is where we want to send the user if we log them out due to an invalid refresh token, but we can't refer to the SplashScreen from outside of this module,

Do we want to send them all the way to the splash screen? Or just the FavoritesActivity?

app/src/main/res/layout/activity_oauth_token_test.xml, line 1 at r5 (raw file):

<?xml version="1.0" encoding="utf-8"?>

remove

app/src/main/res/menu/popup_menu.xml, line 8 at r5 (raw file):

			android:title="@string/logoutMenuItemText"
			android:visible="false" />
	<item android:id="@+id/test_oauth"

remove

support/src/main/AndroidManifest.xml, line 37 at r5 (raw file):

				android:windowSoftInputMode="adjustResize">

			<intent-filter android:label="Redirect from CAS">

Can't we remove this now?

support/src/main/java/edu/byu/support/feature/FeatureLoader.java, line 38 at r5 (raw file):

 */

public class FeatureLoader extends AsyncTaskLoader<FeaturesWrapper> implements ByuCallManager {

I think we were only implementing this because we didn't have a context... Is that true? Actually... I might be wrong... look into this one

support/src/main/java/edu/byu/support/fragment/ByuCameraFeedFragment.java, line 39 at r5 (raw file):

	private ScheduledExecutorService scheduledExecutorService;
	private boolean running = false;
	private final ByuCallback<ResponseBody> callback = new ByuCallback<ResponseBody>(this) {

Reason why this isn't inlin anymore?

support/src/main/java/edu/byu/support/login/LoginWebViewActivity.java, line 44 at r5 (raw file):

public class LoginWebViewActivity extends AccountAuthenticatorActivity {
	public static final String SANDBOX_TOKEN = "sandboxToken";

Maybe unneeded?

support/src/main/java/edu/byu/support/login/LoginWebViewActivity.java, line 45 at r5 (raw file):

public class LoginWebViewActivity extends AccountAuthenticatorActivity {
	public static final String SANDBOX_TOKEN = "sandboxToken";
	public static final boolean USE_SANDBOX = false;

This should be private in the end (after you've made your changes)

support/src/main/java/edu/byu/support/login/LoginWebViewActivity.java, line 63 at r5 (raw file):

		configureWebView(webView);
		loginWithSandbox = getIntent().getBooleanExtra(SANDBOX_TOKEN, USE_SANDBOX);

Instead of getting an extra, this can probably just be saved as a private static final variable at the top. Then people only have to change one place

support/src/main/java/edu/byu/support/login/LoginWebViewActivity.java, line 65 at r5 (raw file):

		loginWithSandbox = getIntent().getBooleanExtra(SANDBOX_TOKEN, USE_SANDBOX);
		String androidId = Settings.Secure.getString(getContentResolver(), Settings.Secure.ANDROID_ID);
		if (loginWithSandbox) {

Use ternary to simplify and remove duplicate code
webView.loadUrl(String.format(".....", loginWithSandbox ? SANDBOX_ID : PROD_ID, androidId);

support/src/main/java/edu/byu/support/login/LoginWebViewActivity.java, line 66 at r5 (raw file):

		String androidId = Settings.Secure.getString(getContentResolver(), Settings.Secure.ANDROID_ID);
		if (loginWithSandbox) {
			webView.loadUrl("https://api.byu.edu/authorize?response_type=code&client_id=" + SANDBOX_CLIENT_ID + "&redirect_uri=byuSuite://auth&scope=openid%20device_" + androidId);

Use String.format() instead of concatenation

support/src/main/java/edu/byu/support/login/LoginWebViewActivity.java, line 94 at r5 (raw file):

				// When we get the token, it has an expires_in but not an expire_date. setupExpireDate uses the current time and expires_in to set a expireDate field
				String accessToken = new Gson().toJson(token.setupExpireDate());
				String[] splitToken = token.getIdToken().split("\\.");

Let's move this stuff into the token class. Maybe a function called getNetId()

support/src/main/java/edu/byu/support/login/LoginWebViewActivity.java, line 97 at r5 (raw file):

				String decodedJwt = new String(Base64.decode(splitToken[1], Base64.DEFAULT));
				String username = decodedJwt.split("\"net_id\":")[1].split("\"")[1];
				setResult(username, accessToken);

You would want the token to have a isSandbox or isProduction field that you'd set before this line

support/src/main/java/edu/byu/support/login/LoginWebViewActivity.java, line 125 at r5 (raw file):

		accountManager.addAccountExplicitly(account, null, null);
		if (loginWithSandbox) {
			accountManager.setAuthToken(account, AuthConstants.TYPE_BEARER_TOKEN_SANDBOX, token);

I don't think we need to differentiate between these two. Just save TYPE_BEARER_TOKEN

support/src/main/java/edu/byu/support/login/LoginWebViewActivity.java, line 140 at r5 (raw file):

	}

	public void finish(ProgressDialog progressDialog) {

Unless we use this multiple times, inline this function

support/src/main/java/edu/byu/support/login/LoginWebViewActivity.java, line 175 at r5 (raw file):

	}

	@SuppressLint("SetJavaScriptEnabled") //It's okay because we only open urls that we (byu) own

Is this needed? Does the login need javascript? We might not need any of this function except the last line... Comment out and see what can be removed

support/src/main/java/edu/byu/support/login/LoginWebViewActivity.java, line 203 at r5 (raw file):

			}
			view.loadUrl(url);
			return true;

I don't want to tempt luck, but I think instead of calling view.loadUrl(url) and then returning true, we can just return false and it will load for us... Try it. See what happens. If it works, put it in the function above as well

support/src/main/java/edu/byu/support/login/LoginWebViewActivity.java, line 206 at r5 (raw file):

		}

		private boolean handledExternalUrl(String url) {

Rename to handledAuthCodeUrl

support/src/main/java/edu/byu/support/login/authentication/AccountManagerHelper.java, line 29 at r5 (raw file):

	public static String getBearerTokenFromAccountManagerBundle(Bundle bundle) throws IOException {
		return buildBearerTokenAuthentication(OAuthToken.deserialize(getNonNullAuthTokenFromBundle(bundle)));

Why did this change? Did we remove the deserialize function from OAuthToken?

support/src/main/java/edu/byu/support/login/authentication/clients/wso2/OAuthToken.java, line 20 at r5 (raw file):

	private String accessToken;
	private Long expireDate;

So in here we need to save one more variable isSandbox or something like that

support/src/main/java/edu/byu/support/login/authentication/clients/wso2/OAuthToken.java, line 78 at r5 (raw file):

	}

	public String serialize() {

Hmm.. Why did we remove these? I'm okay with it, but I'm curious

support/src/main/java/edu/byu/support/login/authentication/clients/wso2/Wso2Client.java, line 22 at r5 (raw file):

    @Override
    protected String getBaseUrl() { return "https://wl4pekqqac.execute-api.us-west-2.amazonaws.com/"; }

Add new lines again (all functions in here - let Android Studio collapse them for you)

support/src/main/java/edu/byu/support/login/authentication/clients/wso2/Wso2Client.java, line 38 at r5 (raw file):

        @POST("PROD")
        @Headers("Accept: application/json")
        Call<OAuthToken> getOAuthToken();

Did you change your preferences to spaces instead of tabs? It looks like this file got reformatted incorrectly...

support/src/main/java/edu/byu/support/login/authentication/providers/ByuAccountAuthenticator.java, line 31 at r5 (raw file):

 */
public class ByuAccountAuthenticator extends AbstractAccountAuthenticator implements AuthConstants {
	public final static String BAD_REFRESH_TOKEN = "_INVALID_REFRESH_TOKEN_";

Why did we make this one again? I remember it was a hack, but I can't remember why we did it...

support/src/main/java/edu/byu/support/login/authentication/providers/ByuAccountAuthenticator.java, line 67 at r5 (raw file):

			} else {
				// Otherwise return an intent to start the login activity for them to put in their credentials
				result.putParcelable(AccountManager.KEY_INTENT, new Intent(context, LoginActivity.class).putExtra(AccountManager.KEY_ACCOUNT_AUTHENTICATOR_RESPONSE, response));

Why did this change so much? I think it should look very similar to the previous method, just with the LoginWebViewActivity

support/src/main/java/edu/byu/support/login/authentication/providers/ByuAccountAuthenticator.java, line 123 at r5 (raw file):

					}
				}, 0);
			case TYPE_BEARER_TOKEN_SANDBOX:

We should be able to delete this completely (are there any differences between these two?)

support/src/main/java/edu/byu/support/login/authentication/providers/ByuAccountAuthenticator.java, line 145 at r5 (raw file):

					}, 0);
				} else {
					return null; // is this necessary?

Probably not

support/src/main/java/edu/byu/support/login/mpn/RegisterWithMpnService.java, line 126 at r5 (raw file):

		// If multiple calls have been made with an invalid refresh token, each one will try to log the user out which causes problems if a previous call has already done so,
		// so we check for an active account here before logging them out.
		} else if (AccountResolver.getAccount(this) != null) { // If they haven't registered before just log them out and stop the service

Was this to fix an error?

support/src/main/java/edu/byu/support/retrofit/ByuApiGenerator.java, line 64 at r5 (raw file):

				builder.addInterceptor(new PublicBearerTokenInterceptor(context));
				break;
			case BEARER_TOKEN_SANDBOX:

Remove. Literally remove every reference to this. It should be gone

support/src/main/java/edu/byu/support/retrofit/ByuApiGenerator.java, line 103 at r5 (raw file):

		public Response intercept(Chain chain) throws IOException {
			if (AccountResolver.getAccount(context) != null) {
				return addBearerToken(chain, context, LoginWebViewActivity.USE_SANDBOX);

I think this won't be needed anymore. We will only need the first two parameters. Is this correct?

support/src/main/java/edu/byu/support/retrofit/ByuCallback.java, line 28 at r5 (raw file):

 */
public abstract class ByuCallback<T> implements Callback<T> {
	public static Class rootActivity;

Comment here (very similar to the one when you set it). Just explain why this is needed and how it's a known hack

support/src/main/java/edu/byu/support/retrofit/ByuCallback.java, line 46 at r5 (raw file):

	}

	public ByuCallback(SimpleCallManager callManager) {

We need javadocs on each of these. I think you instead put the javadoc on the SimpleCallManager class. I need them here

support/src/main/java/edu/byu/support/retrofit/ByuCallback.java, line 98 at r5 (raw file):

			retryCount--;
		} else {
 			if (throwable != null) {

Definitely  a comment here. Also, double check that all of this is needed and works as we think

support/src/main/java/edu/byu/support/retrofit/ByuClient.java, line 20 at r5 (raw file):

	protected ByuApiGenerator.AuthType getAuthType() {
		if (LoginWebViewActivity.USE_SANDBOX) {
			return ByuApiGenerator.AuthType.BEARER_TOKEN_SANDBOX;

Remove

support/src/main/java/edu/byu/support/webview/ByuWebViewActivity.java, line 47 at r5 (raw file):

		// If they're not logged in or if we fail to get a cookie, the webview will be redirected to cas
		// that's inconvenient, so we try to get a cookie, but it doesn't break anything, so we don't try too hard
//		Account account = AccountResolver.getAccount(this);

Remove?

support/src/main/res/layout/login_activity.xml, line 7 at r5 (raw file):

	android:layout_width="match_parent"
	android:layout_height="match_parent"
	tools:context="edu.byu.support.login.LoginWebViewActivity">

I don't think we'll need this xml file anymore... What is this doing?

support/src/main/res/values/strings.xml, line 6 at r5 (raw file):

	<string name="empty_string" />
	<string name="feature_icon_description">Icon representing the feature</string>
	<string name="bad_refresh_token">_INVALID_REFRESH_TOKEN_</string>

Did we define this twice? I thought I already saw this as a public static final somewhere...

support/src/main/res/values/strings.xml, line 30 at r5 (raw file):

	<!--LoginWebViewActivity-->
	<string name="netIdHint">Net ID</string>

All these should be unneeded now

Comments from Reviewable

Review status: all files reviewed at latest revision, 45 unresolved discussions.

app/src/main/AndroidManifest.xml, line 90 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Remove (in the future, make sure to always use git add -p and only ever add the files that are "dev" ready)

Ok, my bad.  I wanted to be able to keep using this in case any more issues came up during this process.  In the future what would be the best way to keep stuff like this from being deleted without including it in the pull request?

app/src/main/java/edu/byu/suite/features/addDrop/controller/mySchedule/EnrolledFragment.java, line 11 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Remove. Also, remove from all other files. I'm assuming this got added when you used the cast. Now that we got rid of that, we should also be able to get rid of this

Done.

app/src/main/java/edu/byu/suite/features/addDrop/service/ClassesClient.java, line 37 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Whoa... What did we do here? Did you make BEARER_TOKEN the default or something?

I did leave BEARER_TOKEN as the default.  Should I change it back to being an abstract method?

app/src/main/java/edu/byu/suite/features/myFinancialCenter/controller/UnpaidChargesFragment.java, line 85 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Was this fixing a bug?

Yeah it was, shoot I thought I added a comment here.  I just added one.

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.java, line 122 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Umm... Why did this get inlined?

I think I may have changed it because I thought it was strange for the Callback to be a member variable instead of inlined like it usually is, but I don't think there was any reason besides that.  Was there a reason it was declared as a member variable?

app/src/main/java/edu/byu/suite/mainScreen/SplashScreen.java, line 14 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Do we want to send them all the way to the splash screen? Or just the FavoritesActivity?

Oh right!  I needed to talk to you about this, when an activity has made multiple calls to different clients with an invalid refresh token, it wouldn't log the user out correctly when we sent them just back to the FavoritesActivity, so this was a solution I found.  I'll show you exactly what happens when you get back.

support/src/main/AndroidManifest.xml, line 37 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Can't we remove this now?

Done.

support/src/main/java/edu/byu/support/fragment/ByuCameraFeedFragment.java, line 39 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Reason why this isn't inlin anymore?

I had changed it because when I was passing in a ByuActivity instead of a ByuFragment the ByuActivity was null at this point.  Should I change this back?

support/src/main/java/edu/byu/support/login/LoginWebViewActivity.java, line 44 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Maybe unneeded?

Done.

support/src/main/java/edu/byu/support/login/LoginWebViewActivity.java, line 45 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
This should be private in the end (after you've made your changes)

Done.

support/src/main/java/edu/byu/support/login/LoginWebViewActivity.java, line 97 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
You would want the token to have a isSandbox or isProduction field that you'd set before this line

Done.

support/src/main/java/edu/byu/support/login/LoginWebViewActivity.java, line 125 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
I don't think we need to differentiate between these two. Just save TYPE_BEARER_TOKEN

Done.

support/src/main/java/edu/byu/support/login/authentication/AccountManagerHelper.java, line 29 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Why did this change? Did we remove the deserialize function from OAuthToken?

Yeah, you had sent me a Slack message saying I should use Json instead of String deserialization.

support/src/main/java/edu/byu/support/login/authentication/clients/wso2/OAuthToken.java, line 20 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
So in here we need to save one more variable isSandbox or something like that

Done.

support/src/main/java/edu/byu/support/login/authentication/clients/wso2/OAuthToken.java, line 78 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Hmm.. Why did we remove these? I'm okay with it, but I'm curious

Same reason as above.

support/src/main/java/edu/byu/support/login/authentication/providers/ByuAccountAuthenticator.java, line 123 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
We should be able to delete this completely (are there any differences between these two?)

Done.

support/src/main/java/edu/byu/support/login/authentication/providers/ByuAccountAuthenticator.java, line 145 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Probably not

Done.

support/src/main/java/edu/byu/support/login/mpn/RegisterWithMpnService.java, line 126 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Was this to fix an error?

Yes.

support/src/main/java/edu/byu/support/retrofit/ByuApiGenerator.java, line 64 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Remove. Literally remove every reference to this. It should be gone

Done.

support/src/main/java/edu/byu/support/retrofit/ByuApiGenerator.java, line 103 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
I think this won't be needed anymore. We will only need the first two parameters. Is this correct?

Done.

support/src/main/java/edu/byu/support/retrofit/ByuClient.java, line 20 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Remove

Done.

support/src/main/java/edu/byu/support/webview/ByuWebViewActivity.java, line 47 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Remove?

Done.

Comments from Reviewable

Reviewed 1 of 26 files at r2, 1 of 51 files at r4, 1 of 18 files at r5, 13 of 13 files at r6.
Review status: all files reviewed at latest revision, 32 unresolved discussions.

app/src/main/AndroidManifest.xml, line 90 at r5 (raw file):
Previously, geogor37 (Geoff Gorham) wrote…
Ok, my bad.  I wanted to be able to keep using this in case any more issues came up during this process.  In the future what would be the best way to keep stuff like this from being deleted without including it in the pull request?

No worries :) Normally, I just leave it as not added. git add -p allows you to add only what you want.

app/src/main/java/edu/byu/suite/features/myFinancialCenter/controller/UnpaidChargesFragment.java, line 90 at r6 (raw file):

					showErrorDialog(byuError.getReadableMessage(), (dialog, which) -> loadUnpaidCharges());
				}
				showErrorDialog(byuError.getReadableMessage());

Should this be in an else? It seems like this code would display two dialogs...

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.java, line 122 at r5 (raw file):
Previously, geogor37 (Geoff Gorham) wrote…
I think I may have changed it because I thought it was strange for the Callback to be a member variable instead of inlined like it usually is, but I don't think there was any reason besides that.  Was there a reason it was declared as a member variable?

Yup. If we define it at the top as a private static final, the compiler is able to optimize it (because this function is called multiple times)

app/src/main/java/edu/byu/suite/mainScreen/SplashScreen.java, line 14 at r5 (raw file):
Previously, geogor37 (Geoff Gorham) wrote…
Oh right!  I needed to talk to you about this, when an activity has made multiple calls to different clients with an invalid refresh token, it wouldn't log the user out correctly when we sent them just back to the FavoritesActivity, so this was a solution I found.  I'll show you exactly what happens when you get back.

Thanks. I'll leave this unresolved

support/src/main/AndroidManifest.xml, line 37 at r5 (raw file):
Previously, geogor37 (Geoff Gorham) wrote…
Done.

This doesn't look removed yet... Did it not get pushed?

support/src/main/java/edu/byu/support/fragment/ByuCameraFeedFragment.java, line 39 at r5 (raw file):
Previously, geogor37 (Geoff Gorham) wrote…
I had changed it because when I was passing in a ByuActivity instead of a ByuFragment the ByuActivity was null at this point.  Should I change this back?

Yes please (for the same reason as the other one)

support/src/main/java/edu/byu/support/login/LoginWebViewActivity.java, line 44 at r5 (raw file):
Previously, geogor37 (Geoff Gorham) wrote…
Done.

This one hasn't been removed yet. Is there a reason we still need it?

support/src/main/java/edu/byu/support/login/LoginWebViewActivity.java, line 45 at r5 (raw file):
Previously, geogor37 (Geoff Gorham) wrote…
Done.

So if I understand correctly, this will be the variable someone will change to switch to use sandbox? If so, let's put a comment here explaining that.

support/src/main/java/edu/byu/support/login/authentication/clients/wso2/OAuthToken.java, line 14 at r6 (raw file):

	private String accessToken;
	private Long expireDate;
	private boolean isSandbox;

Let's just name this sandbox. That way the getter function will be isSandbox() and the setter will be setSandbox()

support/src/main/java/edu/byu/support/retrofit/ByuApiGenerator.java, line 33 at r6 (raw file):

		// BEARER_TOKEN_PUBLIC has the same behavior as BEARER_TOKEN when a user is logged in. If no one is logged in, it gets a generic bearer token not associated with any user
		// NONE does nothing
		BEARER_TOKEN, BEARER_TOKEN_PUBLIC, NONE

Talk to me about BEARER_TOKEN_PUBLIC. There might be a way for us to delete it

Comments from Reviewable

Review status: 53 of 61 files reviewed at latest revision, 33 unresolved discussions.

app/src/main/AndroidManifest.xml, line 90 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
No worries :) Normally, I just leave it as not added. git add -p allows you to add only what you want.

So is there a way to uncommit this?  Or should I just wait to remove it until after we're finished with the review process?

support/src/main/AndroidManifest.xml, line 37 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
This doesn't look removed yet... Did it not get pushed?

Now it should be removed.

support/src/main/java/edu/byu/support/fragment/ByuCameraFeedFragment.java, line 39 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Yes please (for the same reason as the other one)

Done.

support/src/main/java/edu/byu/support/login/LoginWebViewActivity.java, line 44 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
This one hasn't been removed yet. Is there a reason we still need it?

Nope, it's gone now.

support/src/main/java/edu/byu/support/login/LoginWebViewActivity.java, line 45 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
So if I understand correctly, this will be the variable someone will change to switch to use sandbox? If so, let's put a comment here explaining that.

Done.

support/src/main/java/edu/byu/support/login/LoginWebViewActivity.java, line 63 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Instead of getting an extra, this can probably just be saved as a private static final variable at the top. Then people only have to change one place

Done.

support/src/main/java/edu/byu/support/login/LoginWebViewActivity.java, line 65 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Use ternary to simplify and remove duplicate code
webView.loadUrl(String.format(".....", loginWithSandbox ? SANDBOX_ID : PROD_ID, androidId);

Done.

support/src/main/java/edu/byu/support/login/LoginWebViewActivity.java, line 66 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Use String.format() instead of concatenation

Done.

support/src/main/java/edu/byu/support/login/LoginWebViewActivity.java, line 94 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Let's move this stuff into the token class. Maybe a function called getNetId()

Done.

support/src/main/java/edu/byu/support/login/LoginWebViewActivity.java, line 140 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Unless we use this multiple times, inline this function

Done.

support/src/main/java/edu/byu/support/login/LoginWebViewActivity.java, line 206 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Rename to handledAuthCodeUrl

Done.

support/src/main/java/edu/byu/support/login/authentication/clients/wso2/OAuthToken.java, line 14 at r6 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Let's just name this sandbox. That way the getter function will be isSandbox() and the setter will be setSandbox()

Done.

support/src/main/java/edu/byu/support/login/authentication/clients/wso2/Wso2Client.java, line 22 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Add new lines again (all functions in here - let Android Studio collapse them for you)

Done.

support/src/main/java/edu/byu/support/login/authentication/clients/wso2/Wso2Client.java, line 38 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Did you change your preferences to spaces instead of tabs? It looks like this file got reformatted incorrectly...

Done.

support/src/main/java/edu/byu/support/login/authentication/providers/ByuAccountAuthenticator.java, line 31 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Why did we make this one again? I remember it was a hack, but I can't remember why we did it...

This is the message that gets displayed when a user tries to use an invalid refresh token.  Basically we just created it so we had a consistent message to compare to when we get the IOException before handling the error.

support/src/main/java/edu/byu/support/login/authentication/providers/ByuAccountAuthenticator.java, line 67 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Why did this change so much? I think it should look very similar to the previous method, just with the LoginWebViewActivity

I had changed it to handle sandbox tokens, but we don't need that logic anymore.

support/src/main/java/edu/byu/support/retrofit/ByuCallback.java, line 28 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Comment here (very similar to the one when you set it). Just explain why this is needed and how it's a known hack

Done.

support/src/main/java/edu/byu/support/retrofit/ByuCallback.java, line 46 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
We need javadocs on each of these. I think you instead put the javadoc on the SimpleCallManager class. I need them here

Oh gotcha, I'll do that.

support/src/main/res/layout/login_activity.xml, line 7 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
I don't think we'll need this xml file anymore... What is this doing?

It was the old LoginActivity's xml file.  I just forgot to delete it.

support/src/main/res/values/strings.xml, line 6 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Did we define this twice? I thought I already saw this as a public static final somewhere...

Oh yeah, we had tried defining it here, but then we couldn't access it without a context or something like that.  I guess we just forgot to delete it.

support/src/main/res/values/strings.xml, line 30 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
All these should be unneeded now

Done.

app/src/main/java/edu/byu/suite/features/testingcenter/controller/ScoresFragment.java, line 122 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Yup. If we define it at the top as a private static final, the compiler is able to optimize it (because this function is called multiple times)

Oh gotcha, I'll go ahead and switch that back then

Comments from Reviewable

Review status: 52 of 61 files reviewed at latest revision, 32 unresolved discussions.

app/src/main/java/edu/byu/suite/features/myFinancialCenter/controller/UnpaidChargesFragment.java, line 90 at r6 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Should this be in an else? It seems like this code would display two dialogs...

Yeah I'll go ahead and change that.

support/src/main/java/edu/byu/support/feature/FeatureLoader.java, line 38 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
I think we were only implementing this because we didn't have a context... Is that true? Actually... I might be wrong... look into this one

It looks to me like this is necessary because this class uses functions like enqueueCall that come from the ByuCallManager class.

support/src/main/java/edu/byu/support/login/LoginWebViewActivity.java, line 175 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Is this needed? Does the login need javascript? We might not need any of this function except the last line... Comment out and see what can be removed

The automatic redirect didn't work without the Javascript one, but it looks like the other two aren't necessary.

support/src/main/java/edu/byu/support/login/LoginWebViewActivity.java, line 203 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
I don't want to tempt luck, but I think instead of calling view.loadUrl(url) and then returning true, we can just return false and it will load for us... Try it. See what happens. If it works, put it in the function above as well

Seems to work fine!

support/src/main/java/edu/byu/support/retrofit/ByuCallback.java, line 98 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Definitely  a comment here. Also, double check that all of this is needed and works as we think

It seems to be working fine like this, my only question is what is the purpose of the CookieManager lines?  Taking them out didn't seem to cause any problems, but I'm wondering if the consequences are just less obvious?

Comments from Reviewable

Reviewed 10 of 11 files at r7.
Review status: all files reviewed at latest revision, 21 unresolved discussions.

app/src/main/AndroidManifest.xml, line 90 at r5 (raw file):
Previously, geogor37 (Geoff Gorham) wrote…
So is there a way to uncommit this?  Or should I just wait to remove it until after we're finished with the review process?

Just wait to remove it until the end. Just leave this comment as unfinished

support/src/main/AndroidManifest.xml, line 35 at r7 (raw file):

				android:label="Login"
				android:launchMode="singleTask"
				android:windowSoftInputMode="adjustResize">

Make this self-closing (change ></activity> to  />

support/src/main/java/edu/byu/support/fragment/ByuCameraFeedFragment.java, line 39 at r5 (raw file):
Previously, geogor37 (Geoff Gorham) wrote…
Done.

Make it final again

support/src/main/java/edu/byu/support/fragment/ByuCameraFeedFragment.java, line 13 at r7 (raw file):

import android.widget.TextView;
import edu.byu.support.R;
import edu.byu.support.activity.ByuActivity;

Do we need this import?

support/src/main/java/edu/byu/support/login/LoginWebViewActivity.java, line 44 at r7 (raw file):

public class LoginWebViewActivity extends AccountAuthenticatorActivity {
	// This variable should only be changed when we want to test a feature in sandbox
	private static final boolean USE_SANDBOX = false;

When this is true, could we do something to notify the user that they're logging into sandbox? Maybe change the title to "SANDBOX login"?

support/src/main/java/edu/byu/support/login/LoginWebViewActivity.java, line 62 at r7 (raw file):

		configureWebView(webView);
		loginWithSandbox = USE_SANDBOX;

Instead of saving it, can use just use the final variable?

support/src/main/java/edu/byu/support/login/LoginWebViewActivity.java, line 63 at r7 (raw file):

		configureWebView(webView);
		loginWithSandbox = USE_SANDBOX;
		String androidId = Settings.Secure.getString(getContentResolver(), Settings.Secure.ANDROID_ID);

If androidId is null (which does happen), use "default" (you can use ternary to do this if you want)

support/src/main/java/edu/byu/support/login/LoginWebViewActivity.java, line 161 at r7 (raw file):

	}

	 @SuppressLint("SetJavaScriptEnabled") //It's okay because we only open urls that we (byu) own

Remove excess space (before @ sign)

support/src/main/java/edu/byu/support/login/authentication/clients/wso2/OAuthToken.java, line 16 at r7 (raw file):

	private String accessToken;
	private Long expireDate;
	private boolean sandbox;

Just set to false here

support/src/main/java/edu/byu/support/login/authentication/clients/wso2/OAuthToken.java, line 30 at r7 (raw file):

		this.accessToken = accessToken;
		this.expireDate = expireDate;
		this.sandbox = false;

Remove this (see comment above)

support/src/main/java/edu/byu/support/login/authentication/clients/wso2/OAuthToken.java, line 107 at r7 (raw file):

	public String getNetId() {
		String[] splitToken = idToken.split("\\.");

Let's add some try catches in here in case the idToken is bad...

support/src/main/java/edu/byu/support/login/authentication/clients/wso2/Wso2Client.java, line 38 at r5 (raw file):
Previously, geogor37 (Geoff Gorham) wrote…
Done.

It still look like spaces to me...

support/src/main/java/edu/byu/support/login/authentication/providers/ByuAccountAuthenticator.java, line 31 at r5 (raw file):
Previously, geogor37 (Geoff Gorham) wrote…
This is the message that gets displayed when a user tries to use an invalid refresh token.  Basically we just created it so we had a consistent message to compare to when we get the IOException before handling the error.

And the reason why we did that was because the IOException had nothing expect a message for us to identify this error with? What if we created our own error that extended IOExecption?

support/src/main/java/edu/byu/support/retrofit/ByuCallback.java, line 98 at r5 (raw file):
Previously, geogor37 (Geoff Gorham) wrote…
It seems to be working fine like this, my only question is what is the purpose of the CookieManager lines?  Taking them out didn't seem to cause any problems, but I'm wondering if the consequences are just less obvious?

Talk to me about this one

support/src/main/java/edu/byu/support/retrofit/ByuCallback.java, line 39 at r7 (raw file):

	private int retryCount;

	// Any call that could be compromised by an invalid token must use this constructor in order to ensure proper error handling

Make this a javadoc rather than a regular comment. Talk to me me if you need help doing this

Comments from Reviewable

Review status: 57 of 61 files reviewed at latest revision, 21 unresolved discussions.

support/src/main/AndroidManifest.xml, line 35 at r7 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Make this self-closing (change ></activity> to  />

Done.

support/src/main/java/edu/byu/support/fragment/ByuCameraFeedFragment.java, line 39 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Make it final again

Done.

support/src/main/java/edu/byu/support/fragment/ByuCameraFeedFragment.java, line 13 at r7 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Do we need this import?

Nope.

support/src/main/java/edu/byu/support/login/LoginWebViewActivity.java, line 62 at r7 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Instead of saving it, can use just use the final variable?

Yeah haha, I probably should have noticed that one.

support/src/main/java/edu/byu/support/login/LoginWebViewActivity.java, line 63 at r7 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
If androidId is null (which does happen), use "default" (you can use ternary to do this if you want)

Done.

support/src/main/java/edu/byu/support/login/LoginWebViewActivity.java, line 161 at r7 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Remove excess space (before @ sign)

Done.

support/src/main/java/edu/byu/support/login/authentication/clients/wso2/OAuthToken.java, line 16 at r7 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Just set to false here

Done.

support/src/main/java/edu/byu/support/login/authentication/clients/wso2/OAuthToken.java, line 30 at r7 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Remove this (see comment above)

Done.

Comments from Reviewable

Reviewed 4 of 4 files at r8.
Review status: all files reviewed at latest revision, 14 unresolved discussions.

support/src/main/java/edu/byu/support/login/LoginWebViewActivity.java, line 62 at r7 (raw file):
Previously, geogor37 (Geoff Gorham) wrote…
Yeah haha, I probably should have noticed that one.

Umm... I think this ternary is backwards...
Also, instead of duplicating this code, let's just do it in two lines.
String androidId = Settings.Secure.getString(getContentResolver(), Settings.Secure.ANDROID_ID));
androidId = androidId != null ? androidId : "default";

Comments from Reviewable

Review status: 57 of 61 files reviewed at latest revision, 14 unresolved discussions.

support/src/main/java/edu/byu/support/login/LoginWebViewActivity.java, line 44 at r7 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
When this is true, could we do something to notify the user that they're logging into sandbox? Maybe change the title to "SANDBOX login"?

Done.

support/src/main/java/edu/byu/support/login/LoginWebViewActivity.java, line 62 at r7 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Umm... I think this ternary is backwards...
Also, instead of duplicating this code, let's just do it in two lines.
String androidId = Settings.Secure.getString(getContentResolver(), Settings.Secure.ANDROID_ID));
androidId = androidId != null ? androidId : "default";

Done.

support/src/main/java/edu/byu/support/login/authentication/clients/wso2/OAuthToken.java, line 107 at r7 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Let's add some try catches in here in case the idToken is bad...

Done.

support/src/main/java/edu/byu/support/login/authentication/clients/wso2/Wso2Client.java, line 38 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
It still look like spaces to me...

Hmm... Weird, some of them were spaces and some of them were tabs, should be good now though!

support/src/main/java/edu/byu/support/retrofit/ByuCallback.java, line 98 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Talk to me about this one

We do need these lines in order to keep the user from being logged back in automatically.

support/src/main/java/edu/byu/support/retrofit/ByuCallback.java, line 39 at r7 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Make this a javadoc rather than a regular comment. Talk to me me if you need help doing this

Done.

Comments from Reviewable

Reviewed 4 of 4 files at r9.
Review status: all files reviewed at latest revision, 11 unresolved discussions.

support/src/main/java/edu/byu/support/login/LoginWebViewActivity.java, line 67 at r9 (raw file):

		Toolbar toolbar = (Toolbar) findViewById(R.id.toolbar);
		toolbar.setTitle(String.format("Log In%s", USE_SANDBOX ? " WITH SANDBOX" : ""));
		toolbar.setTitleTextColor(ContextCompat.getColor(this, R.color.white));

I think I'd actually prefer the color changing. You think we could do that instead?

support/src/main/java/edu/byu/support/login/LoginWebViewActivity.java, line 91 at r9 (raw file):

				String username = token.getNetId();
				if (username == null) {
					onFailure(call, new ByuError("Invalid Id Token"));

Double check with me on this one... What would happen if username was null?

support/src/main/java/edu/byu/support/retrofit/ByuCallback.java, line 110 at r9 (raw file):

 			if (throwable != null) {
				// This checks to see if the error was thrown because of an invalid refresh token, if so, then we log the user out
				if (throwable instanceof IOException && callManager instanceof Context && ByuAccountAuthenticator.BAD_REFRESH_TOKEN.equals(throwable.getMessage())) {

Have we look into throwing a different kind of exception?

Comments from Reviewable

Review status: 60 of 61 files reviewed at latest revision, 11 unresolved discussions.

support/src/main/java/edu/byu/support/login/LoginWebViewActivity.java, line 67 at r9 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
I think I'd actually prefer the color changing. You think we could do that instead?

Done.  Should I leave the title changed as well?

Comments from Reviewable

Reviewed 1 of 1 files at r10.
Review status: all files reviewed at latest revision, 11 unresolved discussions.

support/src/main/java/edu/byu/support/login/LoginWebViewActivity.java, line 67 at r9 (raw file):
Previously, geogor37 (Geoff Gorham) wrote…
Done.  Should I leave the title changed as well?

Nah. You can change that back

Comments from Reviewable

Review status: all files reviewed at latest revision, 11 unresolved discussions.

support/src/main/java/edu/byu/support/login/LoginWebViewActivity.java, line 67 at r9 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Nah. You can change that back

Done.

support/src/main/java/edu/byu/support/login/LoginWebViewActivity.java, line 91 at r9 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Double check with me on this one... What would happen if username was null?

I tested it with this code, and it currently displays the error dialog with the default message and then kicks the user back out to the FavoritesActivity without logging them in.

support/src/main/java/edu/byu/support/login/authentication/providers/ByuAccountAuthenticator.java, line 31 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
And the reason why we did that was because the IOException had nothing expect a message for us to identify this error with? What if we created our own error that extended IOExecption?

We figured out this wouldn't work.  I went ahead and added a comment explaining everything we found out.

support/src/main/java/edu/byu/support/retrofit/ByuApiGenerator.java, line 33 at r6 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Talk to me about BEARER_TOKEN_PUBLIC. There might be a way for us to delete it

Talked to you, and we decided that we don't want to delete it.

support/src/main/java/edu/byu/support/retrofit/ByuCallback.java, line 110 at r9 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Have we look into throwing a different kind of exception?

See comment above.

Comments from Reviewable

Reviewed 2 of 2 files at r11.
Review status: all files reviewed at latest revision, 7 unresolved discussions.

support/src/main/java/edu/byu/support/login/LoginWebViewActivity.java, line 68 at r11 (raw file):

		toolbar.setTitle("Log In");
		toolbar.setTitleTextColor(ContextCompat.getColor(this, R.color.white));
		if (USE_SANDBOX) {

Comment saying "Make an annoying red bar so that the developer knows they're logging into sandbox mode

Comments from Reviewable

Review status: 53 of 61 files reviewed at latest revision, 7 unresolved discussions.

app/src/main/java/edu/byu/suite/mainScreen/SplashScreen.java, line 14 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Thanks. I'll leave this unresolved

Done.

support/src/main/java/edu/byu/support/login/LoginWebViewActivity.java, line 68 at r11 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Comment saying "Make an annoying red bar so that the developer knows they're logging into sandbox mode

Done.

Comments from Reviewable

Reviewed 8 of 8 files at r12.
Review status: all files reviewed at latest revision, 8 unresolved discussions.

support/src/main/java/edu/byu/support/retrofit/ByuApiGenerator.java, line 103 at r12 (raw file):

			Bundle result = AccountManager.get(context).getAuthToken(AccountResolver.getAccount(context), AuthConstants.TYPE_BEARER_TOKEN, null, false, null, null).getResult();
			return chain.proceed(chain.request().newBuilder().addHeader("Authorization", AccountManagerHelper.getBearerTokenFromAccountManagerBundle(result)).build());
		} catch (OperationCanceledException | AuthenticatorException | IllegalArgumentException e) {

Do we still need this? What made us add it?

support/src/main/java/edu/byu/support/retrofit/ByuCallback.java, line 66 at r12 (raw file):

	// This constructor handles the Callback needed for the UnpaidChargesFragment
	public ByuCallback(ByuFragment fragment, String defaultErrorMessage, String pathToReadableError) {

Clean up constructors

support/src/main/java/edu/byu/support/retrofit/ByuCallback.java, line 142 at r12 (raw file):

		Log.e(TAG, "onFailure: ", t);
		// We've observed that when the user has an invalid refreshToken the callManager will sometimes be null, so we check for that here
		if (!call.isCanceled() && callManager != null) {

When could the callManager be null? Every constructor requires one...

Comments from Reviewable

Review status: 53 of 60 files reviewed at latest revision, 8 unresolved discussions.

support/src/main/java/edu/byu/support/retrofit/ByuApiGenerator.java, line 103 at r12 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Do we still need this? What made us add it?

We do because when we log the user out because of an invalid refresh token if they try to open another feature after they've been kicked back out to the FavoritesActivity but before the MpnService has finished logging them out, this will throw an error.

support/src/main/java/edu/byu/support/retrofit/ByuCallback.java, line 66 at r12 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Clean up constructors

Done.  I figured I would just put all the logic in the final constructor, rather than duplicating code throughout all of them.  Let me know if you want me to do this differently.

support/src/main/java/edu/byu/support/retrofit/ByuCallback.java, line 142 at r12 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
When could the callManager be null? Every constructor requires one...

The only time this has happened for me is trying to get into the Testing Center feature with an invalid refresh token, and it seems like this is maybe an issue of the Activity/Fragment getting destroyed before getting to this point, but I'm not totally sure.

Comments from Reviewable

Reviewed 8 of 8 files at r13.
Review status: all files reviewed at latest revision, 11 unresolved discussions.

support/src/main/AndroidManifest.xml, line 36 at r13 (raw file):

				android:launchMode="singleTask"
				android:windowSoftInputMode="adjustResize" >
			<intent-filter android:label="Redirect from CAS" >

Is this indented properly? Also remove excess space on the end of this and tag above tag

support/src/main/AndroidManifest.xml, line 44 at r13 (raw file):

				<data
					android:host="auth"
					android:scheme="byusuite" />

Swap these (put scheme above host)

support/src/main/java/edu/byu/support/login/LoginActivity.java, line 68 at r13 (raw file):

	public void handleLogin(final Wso2Client.AuthCodeWrapper authCode) {
		final ProgressDialog progressDialog = ProgressDialog.show(LoginActivity.this, null, "Authenticating...", true, false);

No need for LoginActivity before this

support/src/main/java/edu/byu/support/retrofit/ByuApiGenerator.java, line 103 at r12 (raw file):
Previously, geogor37 (Geoff Gorham) wrote…
We do because when we log the user out because of an invalid refresh token if they try to open another feature after they've been kicked back out to the FavoritesActivity but before the MpnService has finished logging them out, this will throw an error.

I just meant if you look at the diff since revision 6, we added a catch that didn't exist before. Do we need that?

support/src/main/java/edu/byu/support/retrofit/ByuCallback.java, line 142 at r12 (raw file):
Previously, geogor37 (Geoff Gorham) wrote…
The only time this has happened for me is trying to get into the Testing Center feature with an invalid refresh token, and it seems like this is maybe an issue of the Activity/Fragment getting destroyed before getting to this point, but I'm not totally sure.

Does testing center not cancel it's calls? Can you recreate this consistently?

support/src/main/java/edu/byu/support/retrofit/ByuCallback.java, line 42 at r13 (raw file):

	/**
	 * Any call that could be compromised by an invalid token must pass in either a ByuActivity or ByuFragment in order to ensure proper error handling; a SimpleCallManager
	 * should only ever be used when the call doesn't require a token.

Doesn't require a token? I don't know if that's correct... They'll still need a token, they just won't handle bad refresh tokens, right?

Comments from Reviewable

Review status: 56 of 61 files reviewed at latest revision, 11 unresolved discussions.

support/src/main/AndroidManifest.xml, line 36 at r13 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Is this indented properly? Also remove excess space on the end of this and tag above tag

Done.

support/src/main/AndroidManifest.xml, line 44 at r13 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Swap these (put scheme above host)

Done.

support/src/main/java/edu/byu/support/login/LoginActivity.java, line 68 at r13 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
No need for LoginActivity before this

Done.

support/src/main/java/edu/byu/support/retrofit/ByuApiGenerator.java, line 103 at r12 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
I just meant if you look at the diff since revision 6, we added a catch that didn't exist before. Do we need that?

Right, that's why I added that catch because the error I described above was throwing an IllegalArgumentException that wasn't getting caught before.  I noticed it when I was testing the invalid refresh token handling yesterday.

support/src/main/java/edu/byu/support/retrofit/ByuCallback.java, line 142 at r12 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Does testing center not cancel it's calls? Can you recreate this consistently?

We discovered that the error was due to the ByuCallback constructor being called before the ByuFragment had been attached to a ByuActivity.  I fixed this and added a comment to the ByuCallback Javadoc about it.

support/src/main/java/edu/byu/support/retrofit/ByuCallback.java, line 42 at r13 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Doesn't require a token? I don't know if that's correct... They'll still need a token, they just won't handle bad refresh tokens, right?

Yeah, I'll rephrase that so it's a little clearer.

Comments from Reviewable

Reviewed 1 of 8 files at r13, 5 of 5 files at r14.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

Comments from Reviewable

Review status: all files reviewed at latest revision, 5 unresolved discussions.

app/src/main/AndroidManifest.xml, line 90 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Just wait to remove it until the end. Just leave this comment as unfinished

Done.

app/src/main/java/edu/byu/suite/mainScreen/FavoritesActivity.java, line 215 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Remove

Done.

app/src/main/java/edu/byu/suite/mainScreen/OAuthTokenTestActivity.java, line 1 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
Remove

Done.

app/src/main/res/layout/activity_oauth_token_test.xml, line 1 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
remove

Done.

app/src/main/res/menu/popup_menu.xml, line 8 at r5 (raw file):
Previously, romrell4 (Eric Romrell) wrote…
remove

Done.

Comments from Reviewable


node rhsplib getADC is missing parameters

This will be a breaking change as we fix this.

I actually don't think this should be considered a breaking change. It's highly unlikely that anyone is calling a function that is currently 100% useless.


Remove loadMetadataCommand

What changes are proposed in this pull request?
Remove loadMetadataCommand.
Why are the changes needed?
Remove loadMetadataCommand.
It is not used in new arch.
Does this PR introduce any user facing changes?
NA

i already merged my golang change; could you also remove them as part of this PR? #18016

Sure, will do that once I get confirmation from @elega

@jja725 Please help double confirm, the load is required.

going to force merge since this is a known issue (and the test suite only runs 3 tests) rather than wasting a cycle to rebase

alluxio-bot, force-merge this please


Sample doesn't work on Edge

This shows a blank page on Edge: https://emilk.github.io/egui/index.html
Should probably at least show some kind of error message if it isn't supported on Edge (or better yet, fix whatever prevents it from working).

Thanks for the report!
I don't have a Windows machine, so it is hard for me to debug. Does the developer console say anything helpful?

It says:

RuntimeError: unreachable
at __rust_start_panic (:wasm-function[1651]:0xe9459)
at rust_panic (:wasm-function[1330]:0xe7dde)
at std::panicking::rust_panic_with_hook::hc3afc981c0e7d6a8 (:wasm-function[998]:0xde523)
at std::panicking::begin_panic_handler::{{closure}}::hefb02c9f4c7b9e72 (:wasm-function[1290]:0xe76d3)
at std::sys_common::backtrace::__rust_end_short_backtrace::h3f57de3f451b589e (:wasm-function[1300]:0xe789c)
at rust_begin_unwind (:wasm-function[1259]:0xe705c)
at core::panicking::panic_fmt::he8b4f848efb7b83f (:wasm-function[1301]:0xe78d2)
at core::panicking::panic::h54d056b62390c882 (:wasm-function[1249]:0xe6dc9)
at egui_web::webgl::Painter::new::h6e457df76159f1af (:wasm-function[228]:0x58ba8)
at egui_web::backend::WebBackend::new::h280b638dcbcdba4a (:wasm-function[1003]:0xde8e7)

So I guess something unreachable in webgl is in fact reached? Sorry, web stuff is very much outside my wheel house so I'm not sure how to debug stuff like this.

Update: this went away after restarting Edge. So my hypothesis is that something in my browser session (probably up for days) caused a GPU hang in the past, which (I believe) causes edge to disable using the GPU from then on. Which in turn would disable things like WebGL.
So I think this is really more of "show error message if webgl fails to load" rather than an actual persistent issue on Edge.


added enterprise id to auth test

enterprise id is an optional argument from auth test to determine if
you're dealing with an enterprise org

LGTM, thanks.


Update RN Expo Package README and Peer dependency

📦 Pull Request
Update Peer Dependency of Expo Package and README to denote compatible version of react-native-webview
✅ Fixed Issues
n/a
🚨 Test instructions
n/a
⚠ Don't forget to add a semver label!
Please only add one label:

patch: Bug Fix?
minor: New Feature?
major: Breaking Change?
skip-release: It's unnecessary to publish this change.

📦 Published PR as canary version: Canary Versions

:sparkles: Test out this PR locally via:
npm install<EMAIL_ADDRESS>npm install<EMAIL_ADDRESS># or
yarn add<EMAIL_ADDRESS>yarn add<EMAIL_ADDRESS>

:rocket: PR was released in<EMAIL_ADDRESS><EMAIL_ADDRESS>:rocket:


Still there?

I’m considering replicating my bash build scripts in Python. This would be useful. Happy to contribute, but I’d roll my own if you aren’t going to maintain...

Keep watching


How can I use the finetuned model with text-generation-webui or KoboldAI?

How can I utilize a fine-tuned model with text-generation-webui or KoboldAI for generating text? What are the necessary steps to ensure the successful integration of the model with these interfaces, and are there any specific requirements or dependencies I need to be aware of?

I think you'll need to convert it using https://github.com/tloen/alpaca-lora/blob/main/export_hf_checkpoint.py. Let me figure it out and I'll ad it to the readme.

Thanks lad!
Sent from Proton Mail for iOS
On Sat, Mar 25, 2023 at 06:19, Aleksey Smolenchuk @.***> wrote:

I think you'll need to convert it using https://github.com/tloen/alpaca-lora/blob/main/export_hf_checkpoint.py. Let me figure it out and I'll ad it to the readme.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you authored the thread.Message ID: @.***>

I would also like to know this.

Any updates?


⚠ Bofrost.de has degraded performance

In 6024195, Bofrost.de (https://www.bofrost.de) experienced degraded performance:

HTTP code: 200
Response time: 1456 ms

Resolved: Bofrost.de performance has improved in 613c00a after 7 minutes.


Setup a basic State enum for the game

Migrated from https://github.com/bevyengine/bevy_github_ci_template/issues/64
Same rationale as #4
#6  is a good candidate for GameState::Boot.

As for bikeshedding the name. GameState and AppState are popularly used and familiar, but I would argue that in a game that has multiple state types for different things, they're kinda ambiguous names. What does "GameState" mean? Everything is a game state...
An alternative name would be something like Screen or SequenceState. I use Screen in my own template currently.
I'm totally fine with just going with GameState / AppState though, I don't think it'll cause any confusion for a jam-sized game.

As mentioned in #3

We can simplify https://github.com/MiniaczQ/bevy-substate-based-ui down to a menu + credits


[misc] Allow Codecov to fail uploading

Codecov only allows so many uploads for the same commit, so we want to allow failures after that limit is reached.

Codecov Report
Base: 82.31% // Head: 82.25% // Decreases project coverage by -0.05% :warning:

Coverage data is based on head (d8a8fd2) compared to base (0a6bcb9).
Patch has no changes to coverable lines.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #407      +/-   ##
============================================
- Coverage     82.31%   82.25%   -0.06%
+ Complexity      363      362       -1
============================================
  Files            43       43
  Lines          1730     1730
  Branches        303      303
============================================
- Hits           1424     1423       -1
  Misses          180      180
- Partials        126      127       +1

Impacted Files
Coverage Δ

src/main/kotlin/se/kth/spork/base3dm/Pcs.kt
66.66% <0.00%> (-11.12%)
:arrow_down:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.


Alert History Empty State Default Card Size / Shows With Empty Nodes

If nodes does not exist OR length is zero, show empty card state
Subscription Card default height size is 450px, width is 300px
Fix - If no telegram or text message, do not show an extra separator

Configuration with no alert history (and two out of three inputs):

Configuration with alert history (and all inputs):

Alert History Icons and Date Padding preserved even on narrow screens:

Apologies, due to the prioritization of Acala wallets, I forgot about this PR. I will review and make the updates and/or open a different PR by EOD Monday.

good to know@ looks like there are some conflicts so i'd rebase as well.

Opening a new PR


Add missing method Contains and descriptions

Codecov Report
Patch and project coverage have no change.

Comparison is base (3978143) 100.00% compared to head (fe7e785) 100.00%.

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more
Additional details and impacted files
@@            Coverage Diff            @@
##              main        #4   +/-   ##
=========================================
  Coverage   100.00%   100.00%
=========================================
  Files            4         4
  Lines          461       461
=========================================
  Hits           461       461

Flag
Coverage Δ

unittests
100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.


Failed to load nodelet [/kinect2_bridge] of type [kinect2_bridge/kinect2_bridge_nodelet]

I tried:
roslaunch kinect2_bridge kinect2_bridge.launch
but I came aross an error：
[ERROR] [1453460840.163354866]: Failed to load nodelet [/kinect2_bridge] of type [kinect2_bridge/kinect2_bridge_nodelet]: Could not find library corresponding to plugin kinect2_bridge/kinect2_bridge_nodelet. Make sure the plugin description XML file has the correct name of the library and that the library actually exists.
[FATAL] [1453460840.163649816]: Service call failed!
I followed the steps on github,but still failed to open kinect camera .Does anyone know why this happens？
Thanks in advance

Make sure that your workspace is compiled and that it is sourced.

@17Skye17
Hi, I met the same problem! Have you solved it? Could you share the solutions? Thank you very much!

I solved it! sudo apt-get install ros-jade-image-pipeline is ok! But i met another problem, I want to install OPENCL but it can't support Jetson!


Improve the FAQ on how to use ansible vaults with ansible-navigator

This is a better way to use ansible vaults with ansible-navigator, I really didn't like the existing solutions, but actually one of them together with our usage of gpg inspired me, so here's the PR. :)

I don't know how to add a label, but it's an enhancement.


Pull the authoritative UserName from the NameID

What
Grab the value of NameID. It'll be set as a new field on the AssertionInfo
How
saml.go: add a UserName field to AssertionInfo, set it based on the value of NameID
saml_test.go: make sure we pull the right name for UserName
test_constants.go: changed the "Login" value to be differentiated from the email/NameID
xml_constants.go: constants to extract NameID
Why
This is part of the SAML spec

I think we should also support requesting persistent NameIDs, see: https://github.com/russellhaering/gosaml2/blob/master/saml.go#L58
Other valid values seem to include at least urn:oasis:names:tc:SAML:2.0:nameid-format:persistent - which to my way of thinking seems much more useful to a service provider because it doesn't change.

@russellhaering made nameID format configurable in 1a9c674

+1


Menu bar icon invisible when inactive screen

Description
I have a multiple monitor setup - 2 displays and my MacBook Pro's built in display. When working on another display, the menu bars on other displays goes semi-transparent. At this point, the menu bar icon becomes invisible. This can be shown below:
Occurs both on dark mode and light mode.
Inactive

Active

Steps to Reproduce

Click on the desktop of a second configured display
Observe the change shown above

Expected behavior: Some similar but more translucent icon is shown
Actual behavior: Icon goes invisible
Reproduces how often: Always
Versions
SpotMenu version: 1.9.0
MacOS version: 10.14.6 (18G103)

I have this problem. The song title appears empty. I hope it will be fixed as soon as possible.

Does this always happen or for only specific songs?
On June 25, 2020 at 3:03:15 PM, Muzaffer YILDIRIM<EMAIL_ADDRESS>wrote:
I have this problem. The song title appears empty. I hope it will be fixed
as soon as possible.
[image: image]
https://user-images.githubusercontent.com/22274025/85724895-4e9b4080-b6fd-11ea-9ddf-65e1e930960e.png
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/kmikiy/SpotMenu/issues/214#issuecomment-649526547, or
unsubscribe
https://github.com/notifications/unsubscribe-auth/ADMMMJ5SBJ4VRX7OEVNGWCLRYNDI3ANCNFSM4JFCXWFA
.

This always happens with multiple monitors, on every song!

Same error every time and on different monitors.

I have the same issue with my dual monitor setup, would be great if it was like literally every other menu icon and stays on every screen...

Same issue with triple monitor setup, the icon (or icon + song info) is hidden when screen inactive.

I am having the same issue

I am having the same issue

I am having this issue aswell!

same

same

This project hasn't been touched in years now so I'd jump onto another one, personally I use Spotica Menu now as it behaves better between my displays and works with Sonoma (they said Sequoia support is coming soon)


Netty 5.0 swallowing exceptions

Perhaps this is user error, but this is completely unintuitive in my opinion. Just sat here debugging something for a few hours; turns out Netty has been swallowing a bunch of exceptions, even when exceptionCaught was overridden.
The following code produces no output, and the server being connected to gets no data (it does, however, receive a connection).
package com.boltery.bouncer;

import io.netty.bootstrap.Bootstrap;
import io.netty.buffer.ByteBuf;
import io.netty.buffer.Unpooled;
import io.netty.channel.Channel;
import io.netty.channel.ChannelHandlerAdapter;
import io.netty.channel.ChannelHandlerContext;
import io.netty.channel.ChannelInitializer;
import io.netty.channel.ChannelOption;
import io.netty.channel.EventLoopGroup;
import io.netty.channel.nio.NioEventLoopGroup;
import io.netty.channel.socket.SocketChannel;
import io.netty.channel.socket.nio.NioSocketChannel;
import io.netty.handler.codec.MessageToMessageEncoder;

import java.nio.charset.Charset;
import java.util.List;
import java.util.concurrent.TimeUnit;

public class Maine2 {
	public static void main(String[] args) throws Exception {
		final EventLoopGroup loopGroup = new NioEventLoopGroup();
		final Bootstrap bootstrap = new Bootstrap()
				.group(loopGroup)
				.channel(NioSocketChannel.class)
				.handler(new ChannelInitializer<SocketChannel>() {
					@Override
					protected void initChannel(final SocketChannel socketChannel) throws Exception {
						socketChannel.pipeline().addLast(new MessageToMessageEncoder<Object>() {
							@Override
							protected void encode(final ChannelHandlerContext channelHandlerContext, final Object o, final List<Object> list) throws Exception {
								throw new Exception();
							}

							@Override
							public void exceptionCaught(final ChannelHandlerContext ctx, final Throwable cause) throws Exception {
								// never hit...
								cause.printStackTrace();
							}
						});
					}
				})
				.option(ChannelOption.SO_KEEPALIVE, true);

		final Channel ch = bootstrap.connect("<IP_ADDRESS>", 11792).await().channel();

		final ByteBuf buf = Unpooled.buffer();
		buf.retain();
		buf.writeBytes("Hello".getBytes(Charset.forName("UTF-8")));
		ch.writeAndFlush(buf);

		loopGroup.awaitTermination(2, TimeUnit.SECONDS);
	}
}

Am I missing something here? Adding a future listener and checking .isSuccess() works as expected...
ch.writeAndFlush(buf).addListener((future) -> {
	if (future.isSuccess()) {
		System.out.println("Success");
	} else {
		System.err.println("FAILED"); // correctly prints "FAILED" when run
	}
});

However if there isn't a listener, things should crash or exceptionCaught should be called. This is particularly useful in cases where a step shouldn't fail. What if a listener hasn't been added? This makes debugging an absolute nightmare to the point I would never want to use Netty in production.

exceptionCaught(...) is only called for inbound exceptions. All outbound exceptions must by handled in a listener by design. If you always want to have exceptions handled in exceptionCaught(...) just add a ChannelOutboundHandler that will an listener for every outbound operation

@normanmaurer ChannelOutboundHandler was deprecated in 5.0.

And 5.0 was deprecated completly atm. Use 4.1 in which ChannelOutboundHandler was not deprecated. (In 5.0 you would have used ChannelHandler directly)

Am 17.01.2016 um 16:36 schrieb Josh Junon<EMAIL_ADDRESS>@normanmaurer ChannelOutboundHandler was deprecated in 5.0.
—
Reply to this email directly or view it on GitHub.

How is 5.0 "deprecated completely"? Is it not the next Alpha version?

No we not plan to work on 5.0 any time soon but concentrate on 4.0 and 4.1. All none breaking changes were backported to 4.1

Am 17.01.2016 um 17:37 schrieb Josh Junon<EMAIL_ADDRESS>How is 5.0 "deprecated completely"? Is it not the next Alpha version?
—
Reply to this email directly or view it on GitHub.

Sad, I liked the changes that occurred in 5.0.

Can you list which ones and for what reason?

Am 17.01.2016 um 18:05 schrieb Josh Junon<EMAIL_ADDRESS>Sad, I liked the changes that occurred in 5.0.
—
Reply to this email directly or view it on GitHub.

The refactoring to use Java's Executor APIs a little more. Allows better interoperability between different libraries. That was a big one.
Call me crazy, but pipeline nodes should be agnostic to whether the data they're working on is inbound/outbound. Netty 5.0 started to move towards that I feel like.
One step further would be to explicitly specify which part of the duplex stream a pipeline handler should be put on. Having two streams (inbound/outbound) in the same "stack" (pipeline) gets confusing, and doing automagic determination as to which handlers should be called for inbound/outbound data makes the API a bit messier. I imagine it would reduce under-the-hood code as well.


Laravel 10.11.0 Shift

This pull request includes updates for the recent minor version release of Laravel as well as updates to your package dependencies. Feel free to commit any additional changes to the shift-ci-v10.11.0 branch.
Before merging, you need to:

Checkout the shift-ci-v10.11.0 branch
Review all pull request comments for additional changes
Run composer update
Thoroughly test your application (no tests?, no CI?)

:warning: The Shift build servers run PHP 8.1. If you are running a different version of PHP you should run composer update locally to ensure you have the proper versions of your dependencies to reflect your PHP environment.


Edit Makefile for FreeBSD compatibility

#2
@yurivict can you check if the following is correct?

Yes, now it works correctly.


Popup not getting positioned correctly when first opened

Fix is here: https://github.com/MrFrankel/ngx-popper/pull/33

@damo1884 can you provide a way to reproduce this?

@MrFrankel in this demo you can see the popup is not correctly positioned.

Notice that if you scroll the page horizontally the popup gets fixed.

@ktriek thanks for the demo, seems its related to using float.
Ill look into it, thanks

@ktriek sorry this is taken so long, iv'e been away on vacation.
It seems like an issue with popper.js itself, not sure why its miscalculating.
Ill try to investigate further but for now you can either use popperPositionFixed or set
[popperModifiers]="{boundariesElement : 'viewport'}"

@MrFrankel thanks, this workaround(adding popperPositionFixed or set [popperModifiers]="{boundariesElement : 'viewport'}") fixed my problem.


IntanRawIO: Update support of rhs files

This will be adding support for RHX rhs files with a separate header for rhs files. We haven't officially update RHS support since RHS 1.0 and we are on 3.x so this may take a bit of work. We will add tests files soon so that we can properly test this.
TODOS

[x] Add support for supply and aux channels in RHS Turns out that there isn't any from newest docs!
[x] Add support for one-file-per-signal
[x] Add support for one-file-per-channel
[x] Add test files for new formats
[x] Add access to digital channels like we did for rhd
[x]  Try to understand the stim channels to see if we need to make updates
[x] Add channel_number_dict for one-file-per-signal
[x] Add analog-out in the one-file-per-channel-dict function

Neuroconv provenance https://github.com/catalystneuro/neuroconv/issues/789

The test is failing due to timestamp issue.... That means the test file could be corrupt. Not sure how since this is a test file generated from their code. Since it is a binary file I can just read it myself and see if I can figure out what is going. I think for me what I would say is:

let me fully separate out rhs and rhd for this. I agree with you that trying to squish them together might be trying to hack too much. Some duplication is probably a good idea so we can adapt and adjust one without needing to worry about the other.
Figure out why timestamps are messed up (they really shouldn't be).

The test is failing due to timestamp issue.... That means the test file could be corrupt. Not sure how since this is a test file generated from Intan's simulator. Since it is a binary file I can just read it myself and see if I can figure out what is going on. I think for me what I would say is:

let me fully separate out rhs and rhd for this. I agree with you that trying to squish them together might be trying to hack too much. Some duplication is probably a good idea so we can adapt and adjust one without needing to worry about the other.
Figure out why timestamps are messed up (they really shouldn't be).

Sounds good, let me know if you need to upload something again.

@samuelgarcia @alejoe91
This is actually ready for review now. Tests are passing (barring the retest happening right now which should pass). With this PR we now support all formats of Intan (rhd and rhs, traditional + headerless binary formats). If you think it would be beneficial for this release feel free to review now. Otherwise we can wait. @h-mayorquin said he would do another read-over later. So we can either merge sooner and then fix up or wait and tidy more now.
I would prefer squashing this one. I had some fights with typos in the paths to the test files, so there are a lot of junk commits.

Thanks @zm711
This looks good to me! @h-mayorquin if you want to take another look, otherwise we can squash and merge and include this in the release :)

I took a second look, it looks good to me.

Thanks @h-mayorquin!


Add support for Wisconsin

Utilizes the DNR's GIS service you found. This service outputs numeric codes, and uses a couple CSV files as lookup tables. I hard coded the lookup into the JS and special cased Wisconsin. Happy to redo that if you'd prefer it done a different way.
The DNR also uses two separate fields in the GIS response to get the full information -- one field (VEG_TYPE_CODE) gives you a vegetation code, but if that code is 0 then that means it's a body of water and you're supposed to look at the LUC_LEVEL_2_CODE field to determine lake vs stream vs reservoir. I decided to punt on that for now as it significantly reduced the generalizability of the code in LandCover.js.

hi @kbrose. how fantastic!
i've been thinking about where I want to massage land cover details (as in #19). I think what i would like is to do that by subclassing LandCover in LandCover.js for states.
Handling in Text also makes some sense, so if you don't want to make the change to having it happen in a subclass of LandCover, lmk, and I can bring in your changes and refactor later.

Yeah let me try that. It would also let me query multiple fields to get the specific body-of-water description in Wisconsin.

hi @kbrose, i extended the landcoverserver for illinois, i think that can be a patter for you.

@fgregg Updated to extend the LandCoverServer class. I moved the lowercasing to the coverType() function for MI and IL -- one of the descriptions of Wisconsin includes a proper noun:

oak forest and barrens, including jack pine and Hill's scrub oak

so I don't want to lowercase the results of WI.

thanks @kbrose !!


Introduce gnome-shell-extension-pop-shell

Resolves https://github.com/kachick/dotfiles/issues/528
Merge and pivot from https://github.com/kachick/dotfiles/pull/687

Broken GNOME cursor - White square
https://askubuntu.com/questions/1411558/why-mouse-pointer-showing-as-a-square-on-desktop
https://discourse.gnome.org/t/broken-mouse-cursor/9750/4
https://discourse.gnome.org/t/weired-white-square-cursor-for-gdm/9468


Simple double addition bug

What happened?
Between 4.1 and 7.1 any number with .1 as base, added with 0.1 results in infinity (i.e: 5.1 + 0.1 = 5.1999999999).
The intended result should be double (i.e: 5.1 + 0.1 = 5.2).

Steps to reproduce problem

Create double variable
Try to do simple addition using any number before the decimal starting from 4 and keeping the number after the decimal at 1 ( 4.1, 5.1, 6.1, 7.1).
Now add 0.1 (i.e: 5.1 + 0.1)

Additional info
Your code
What code was in the editor, if any, when the failure occurred? You
can paste it in below:
void main() {

   final mappedNumber0 = (5.0 + 0.1);          // 5.1
   final mappedNumber1 = (5.1 + 0.1);          // 5.19999999999
   final mappedNumber2 = (5.2 + 0.1);          // 5.3

   print('$mappedNumber0 | $mappedNumber1 | $mappedNumber2');
}

DartPad's output
Did DartPad print anything to the console pane? If so, paste it below:
5.1 | 5.199999999999999 | 5.3

This is expected for doubles due to how floating point values work in any language (including Dart and JavaScript) which use IEEE-754 64-bit floating point numbers.
You can read more by reading this comment specific to Dart (https://github.com/dart-lang/sdk/issues/47081#issuecomment-911387804) or this guide about What Every Programmer Should Know About Floating-Point Arithmetic.
To learn more about numbers in Dart and Dart on the web, see Numbers in Dart.
Thanks for opening an issue and let us know if you have any more questions :D


opt code for ctor and dedup redefine for Task Callback

去除冗余的process回调定义
去除一些冗余的代码
利用C++11 Object_construction_improvement 精简构造函数
start_with 的prefix参数传引用

The change of process function definition is a mistake.

The feature of C++11 object construction improvement is not supposed to used in this project.

Thank for you work. I’v updated StringUtil module according to suggestion.

Thank for you work. I’v updated StringUtil module according to suggestion.


Add a flag for building debug / release mode

Discuss which one should be assumed by default. Follow cargo and use debug by default or assume release as default since the package is intended for use in external project?

It is probably best to assume debug as default as this behavior is consistent with what cargo does (and it is the behavior of the currently published version of this tool)


q does not quit radio-active

Windows 10 Pro, Cmder, radio-active 2.9.0
Search for radios in France => radio -C fr
Get the list of french radios
Type the ID I want to listen to
Radio starts streaming
Enter q to stop streaming and quit radio-active
Message "No background radios are running!"
New radio-active prompt while radio keeps streaming

That should not be the case, maybe you are trying to quit before the ffplay  process starts playing the station.
let me investigate

No, I try to quit AFTER ffplay process has started playing the station

FYI, it happened also in version 2.8.0

working on it. found the issue

v 2.9.1
q is working fine now


how do i configure mutate?

how do i find the preference in mutate in ubuntu?

i have installed the python-pip requirements for the calculator, i tried ctrl + D and did "1 + 1" it wasn't calculating like Alfred in Mac. what do i do after installing the calculator dependencies?

Type preferences and select preferences page.
The calculator I don't know about her ;(

i think the calculator part, we need to add it manually into the scripts in the preference. lets wait for the owner to reply on how-to use the calculator after installing the dependencies.

Have you do sudo pip install sympy?

Hi,
thanks a lot for this app! It is great and smoth! ;)
I have a simple question, how can I change the translate to a english dictionary?
And I have one problem. I cant configure my hotkey to lunch Mutate with preference. Everythime I change the default Contrl + D to my preference, it just recycle again to the defaulta when I try to lunch Mutate.
Wish you all the best

qdore, yes i have installed sympy, so how do you make the calculator work?

@paternogbc You must press save after you change your hotkey.
About change the translate to a english dictionary, you should modify this file: ~/.config/Mutate/scripts/google_translate/google_translate.py.(It seems to be this)

@shaunstanislaus the calculator should work fine now, didn't it?

Hi, thanks for your reply.
I saved the preferences but still not working. ;(
The app is fantastic but Contrl + D in my keyboard is not so good.
Another questions is how do I setup for automatic start with login, because I have to start Mutate every time.
All the best,
Gustavo


add Da Nang University of Economics

@MyLinh46 I am sorry, the domain due.udn.vn is removed from our trusted domains list because it was abused in the past. If you are the student of this school, please enroll to GitHub Student Developer Program. Then apply for JetBrains educational subscription with your GitHub account (GitHub tab at https://www.jetbrains.com/shop/eform/students).


Adding position template vars to blocks

This makes some common template vars available to the block rendering, making each block aware of its position in the current blocklist (Fixes #76).
Vars: $Pos, $Parent (reference to the containing SiteTree object), $TotalItems, $First, $Last, $FirstLast.

Hey awesome, I just tried it out and it didn't seem to work though. If I place $FirstLast in template it's always "first last", $Pos is always "1". Did you test these? Or maybe there's just an issue with my set up I dunno. If yours is definitely fine I'll do a fresh install etc and test again. Thanks!

That's weird... I didn't write unittests, but am using the template vars myself to add a 'Back to top' link after the third block.
Are you calling BlockArea('somearea') to get the blocks? (not 'Blocks()' or any other getter)?

And (this template code isn't included on all blocks, hence the output skips a few Pos'es):

Is this something that's currently being worked on? We are running into a situation where some of these vars would be nice to have.

This feature is essential, would love to see this added in the near future. I also had the same issue as sheadawson. It seems the variables are conflicting with one another - I changed Pos to Position and it worked. Many thanks.
Thanks


Add custom trigger hashtag for each client display

At present, each client is checking Twitter for a text prompt using the same hashtag. A new tweet with the trigger hashtag (currently, #dalle) will update the image in all client displays. This issue will make the changes that will assign a unique trigger hashtag to each client so that they can be controlled from Twitter independently (but also includes the option to change them all simultaneously).
Implementation:

Move the trigger hashtag from tweets_utils.py to the config yaml files
Add hashtag that will change all displays (probably just keep the default, #dalle)
Update retrieve_tweets_containing_text_prompt function in tweets_utils.py to include an argument for each trigger hashtag

Need to address this this issue  before proceeding to avoid a hacky solution


Encrypt/decrypt sherpa-onnx models

We built an android tts sherpa onnx model for a client and they want me to encrypt the model, so the voice (celebrity of some sort) which was used to fine tune the model won't be used to cause harm. How can I encrypt and decrypt sherpa onnx models. During conversion from standard onnx to sherpa onnx, there is no mechanism for encryption. Can you add this feature? if there is can you point me in the direction?

We use srandard onnx models.
Please search with Google for that.

Okay thanks


Can't edit "LDG Tau" for pyfr in DEMO mode

When I click on the input box for LDG Tau for the pyfr simulation input it does let me enter a new value. Digging down into the elements there is an  tag there. I see when running with the vagrant VM in DEMO mode.

What is the tag and what new value are you trying to enter?

or you just meant there's an html tag there, scrap that question. What's the new value you're entering?

Yes - just an  html tag. I was trying to delete the value and leave it empty but it doesn't even appear as 'clickable' or give me a cursor when I click on it.

Oh I think I know what it is, when was the last time you pulled.
https://github.com/Kitware/HPCCloud/issues/510

All the issues I am reporting on today come from the DEMO mode so I am:

Getting HPCCloud-deploy from github (did that this morning)
Running DEMO=1 vagrant up which I assume gets cumulus, girder and hpccloud from github during provisioning
Going to localhost:8080 in chrome.

Is this another case where the files in dist need to be rebuild via npm and committed? Does the vagrant/ansible provisioning run npm install or build?

it looks like it builds,
https://github.com/Kitware/HPCCloud-deploy/blob/1cc6407d88443f7589bd95ac0ba738b1845e3652/ansible/roles/hpccloud/tasks/main.yml#L49-L55
do you vagrant provision or just up?

vagrant up but doesn't that run the provisioning if it hasn't been done?

So the vagrant VM is using the master branch not the release branch of hpccloud. Is that the issue?  #510 was merged into release.

from the graph master should have the important part of release in it. My understanding is up doesn't always provision, it just starts up the vm?

I will try to run 'vagrant provision' to see if that fixes it.

 I was able to enter/delete values in the input box if I clicked on the question mark to open the help text so I bet it is related to #510.

There's just an absolute positioned element over the box. Merge into master will fix.

Checked and confirmed this was fixed by #517.


Udate canvastools and fix tests

Update canvastools to optimize onRegionMove and fix tests

Yes LGTM but I think you need to rebase and reopen the PR from v2


Use sync.Pool to reduce the number of memory allocations on encode

In this encoding part, we have too many allocations: https://github.com/talariadb/talaria/blob/2a1cffeacb8ffb743c4e4bc2293a780019e31ab9/client/golang/encoding.go#L36
We can optimise it like this(Just an example)
type encoder struct {
    // ... other fields ...
    batchPool *sync.Pool
}

func newEncoder() *encoder {
    e := new(encoder)
    e.batchPool = &sync.Pool{
        New: func() interface{} {
            return &pb.Batch{Events: make([]*pb.Event, 0, batchSize)}
        },
    }
    return e
}

// Encode implements formatter interface
func (e *encoder) Encode(events []Event) *pb.Batch {
    e.Lock()
    defer e.Unlock()

    e.next = 0
    e.dictionary = make(map[string]uint32, len(events))
    e.batch = e.batchPool.Get().(*pb.Batch)
    e.batch.Events = e.batch.Events[:0]

    // Write the events
    for _, ev := range events {
        encoded := e.encodeEvent(ev)
        e.batch.Events = append(e.batch.Events, encoded)
    }

    // Write the interned strings
    e.writeDictionary()
    e.batchPool.Put(e.batch)
    return e.batch
}

You should only release the data once it's no longer used, otherwise you'll end up with corrupt memory.


[SYSTEMML-540] Reduce the number of unknowns in ConvolutionOp

This PR reduces the unknowns during dynamic recompilation by inferring the input's height/width of ConvolutionOp based on its parent's output's height/width.
Additionally, for developer debugging, I have guarded the functionality with the flag INFER_TENSOR_SHAPE_FROM_PARENT_CONV_OP and have added sufficient documentation to explain how these dimensions are inferred :)

@bertholdreinwald @mboehm7 @dusenberrymw can you please review this PR ?

Refer to this link for build results (access rights to CI server needed):
https://sparktc.ibmcloud.com/jenkins/job/SystemML-PullRequestBuilder/2180/Failed Tests: 10SystemML-PullRequestBuilder/org.apache.systemml:systemml: 10org.apache.sysml.test.integration.applications.dml.NaiveBayesDMLTest.testNaiveBayesDml[2]org.apache.sysml.test.integration.applications.pydml.LinearLogRegPyDMLTest.testLinearLogRegPyDml[2]org.apache.sysml.test.integration.applications.pydml.NaiveBayesPyDMLTest.testNaiveBayesPyDml[2]org.apache.sysml.test.integration.functions.quaternary.WeightedDivMatrixMultTest.testWeightedDivMMMultMinusLeftSparseCPorg.apache.sysml.test.integration.functions.quaternary.WeightedDivMatrixMultTest.testWeightedDivMMLeftEpsCanonicalized2SparseCPorg.apache.sysml.test.integration.functions.quaternary.WeightedDivMatrixMultTest.testWeightedDivMMLeftEpsCanonicalized1SparseCPorg.apache.sysml.test.integration.functions.quaternary.WeightedDivMatrixMultTest.testWeightedDivMM4MultMinusLeftSparseCPorg.apache.sysml.test.integration.functions.quaternary.WeightedDivMatrixMultTest.testWeightedDivMMLeftSparseCPorg.apache.sysml.test.integration.functions.quaternary.WeightedDivMatrixMultTest.testWeightedDivMMMultLeftSparseCPorg.apache.sysml.test.integration.functions.quaternary.WeightedDivMatrixMultTest.testWeightedDivMMLeftEpsSparseCP

Refer to this link for build results (access rights to CI server needed):
https://sparktc.ibmcloud.com/jenkins/job/SystemML-PullRequestBuilder/2181/

@niketanpansare In general, it makes sense to be able to use the previous layer's output shape for the next layer's input shape (and the functions in nn do that), so it should be fine to do it here as well.  The bigger issue for me though is that I don't think hacks like this should be necessary.  The shapes can (assuming the engine worked well....) all be computed at compile time, and thus the shape variables that are returned from every function in nn should be able to be reliably used, without any unknown values.  The fact that there are still unknown values to me just signifies that the engine needs to be improved in the general case.

@dusenberrymw Yes, I agree. Constant propagation part of our engine needs more work.

Btw, should we consider moving these shapes from forward to init?

well, without looking into the details - this change seems to hide two size propagation issues of the ConvolutionOp. First, during (basic) dynamic recompilation we have an unconditional compilation scope of a single DAG, so unless there are data-dependent operators such as table, rand(sum(X),1), etc all dimension sizes become known. I would recommend to look at operators like DataGenOp and adapt the same approach. Also note that IPA for size propagation across functions simply reuses the dynamic recompilation infrastructure. So once the size propagation of the ConvolutionOp is fixed, it would implicitly remove the need for dynamic recompilation altogether. Second, I think the ConvolutionOp does also not correctly mark operators for dynamic recompilation. Normally, we mark operators for dynamic recompilation if the exec type is spark (potentially due to unknowns) and dimensions/sparsity is unknown or in forced cp exec type if the dimensiosn are unknown. Neither applies here, because the ConvolutionOp does not support spark operations.

@mboehm7 There are 2 issues here:

ConvolutionOp is not marked for recompilation as they are on CP.
Evaluation of the below expression should have happened at compile-time via constant propagation:

Hout = as.integer(floor((Hin + 2*padh - Hf)/strideh + 1))
Wout = as.integer(floor((Win + 2*padw - Wf)/stridew + 1))

The scenario is as follows: the first convolution precisely knows the inputs and output dimensions (N, K*Hout*Wout) and propagates that. But since we donot have tensor support, we only know  K*Hout*Wout and not Hout and Wout.

yes, my point was that (1) can be easily fixed, and (2) even if the ConvolutionOp has DAGs of operations instead of literals as input parameters, these can be easily evaluated during dynamic recompilation (with an approach similar to the rows/cols parameters of the DatagenOp) - so there is no need to infer them from the parent, which can create problems due to the order to size propagation. Also, I would recommend to remove _cachedParams as it can lead to incorrect sizes and thus incorrect results if a function is called with different parameters and dynamic recompilation needs to actually update the parameters.

Sure, I can remove _cachedParams. But the core problem still exists: 'unnecessary computation of constant shapes in for loop'. There are two possible solutions:

Refactor nn script to move shape computation to init. Please see 1965 jira.
Constant propagation and removing these computation outside the for loop. That is no need for recomputing the DAG of operations during  every iteration via dynamic recompilation.

Without either of these solutions, we are unnecessarily increasing the SystemML overhead. It is possible to easily construct a neural network case where gpu based forward propagation is so fast that recompilation time is overhead.


Missing BridgeDestroy action

The official Asterisk documentation says that BridgeDestroy action should be available: https://wiki.asterisk.org/wiki/display/AST/Asterisk+16+ManagerAction_BridgeDestroy
Am I missing something here or this action actually missing?

Someone should probably go through the documentation and check for missing actions and events.


Problem with scrims when multiple dialogs are opened

What is affected?
Component
Description
When opening multiple dialogs, scrim are not layered properly resulting in dialogs opening in top of each other with no clear way to discern boundaries.

Reproduction
https://lit.dev/playground/#gist=69e00d591947f65c8b9b19860867d08b
Workaround
Workaround proposed by @sorvell :
import {MdDialog} from<EMAIL_ADDRESS>
MdDialog.addInitializer(async (instance: MdDialog) => {
  await instance.updateComplete;
  instance.dialog.prepend(instance.scrim);
  instance.scrim.style.inset = 0;
  instance.scrim.style.zIndex = -1;

  const {getOpenAnimation, getCloseAnimation} = instance;
  instance.getOpenAnimation = () => {
    const animations = getOpenAnimation.call(this);
    animations.container = [...animations.container, ...animations.dialog];
    animations.dialog = [];
    return animations;
  };
  instance.getCloseAnimation = () => {
    const animations = getCloseAnimation.call(this);
    animations.container = [...animations.container, ...animations.dialog];
    animations.dialog = [];
    return animations;
  };
});

Is this a regression?
Yes. This used to work, but now it doesn't.
Affected versions
1.3.0
Browser/OS/Node environment
Chrome/Firefox

As a side note, this issue is even more significant because dialogs can usually be lazy loaded; Using the workaround above means that the code for <md-dialog> will be loaded in the main bundle (unless the code for this fix is imported everywhere where dialogs are used which makes managing development in a multi-dialog app really cumbersome).

Still an issue with version 2.2.0


rate: Bump max value of reservoir_frame_delay up to 512

With the default max keyint of 10s, and common length fades found in content, 256 frames is not sufficient to allow rate control to adapt properly; it is barely even past the GOP boundary.
This will introduce possible overflow problems with log_scale when 2-pass mode is intoduced.
Notes
This fixes horrendous artifacting on some clips, particularily with fades,
Sample file can be found here. It is in xz-compressed y4m form, for @tdaede's convenience.
It can be reproduced easily with: rav1e --output out.ivf --bitrate 500 fades_make_me_sad.y4m.
@tterribe says inter_delay_target should also be clamped to something like 2x the value of reservoir_frame_delay, but I cannot remember the reason (other than it's related to 2-ass), and thus have not included it. I'll loop back on that one...

Coverage remained the same at 83.317% when pulling 2bbf33e0d6886bd9543446bd8119f0ced5b25e1e on dwbuiten:rcclamp into c2f6a1a10f78afb12154e2789a1f519feb1dc4da on xiph:master.

Any reason for not going any higher, e.g. 1024? Where does the improvement fall off?

needs rebase

@tdaede From what I understood (or misunderstood) from @tterribe , I don't think anything past 1.5x max GOP length would be useful... although that does beg the question what happens on 50 or 60 fps clips!

@tdaede Following a talk with with Tim at the watercooler meeting yesterday, I've changed it to just remove the limit entirely.


Comment:Step-Basic-Middleware

It was a little unclear why the hooks all have names like "hook35".
I decided this is probably because they are typically created as anonymous functions, but took a few moments of confusion to figure out that was the reason (and I'm still less than 100% certain -- just a best-guess, but I'm pretty sure that's the main reason).
Might be nice to clarify that up-front, since this is geared toward newcomers to the framework. Coming into this from other frameworks, I think of before-hooks like "validate address" and after-hooks like "update-leaderboard" or something, not "hook10" and "hook33a".

If profilers or debuggers are displaying hook-chains, it might even be nice to have some facility to give anonymous hooks developer-friendly names, if that isn't already on the roadmap.

The hooks were named in a way to related the hook diagrams. "hook35", for example, relates to the box taged with "35" on the diagram.
Typically you name hooks for what they so. So some of the feathers-hooks-common are named 'setCreatedAt', 'populate', etc.

Ah, that makes sense. In the future, it might improve absorption for newcomers to the framework to have a diagram and code with typical hook names, rather than numbers. I'd volunteer to do this myself but I'm still getting a feel for the framework; not sure if I'm far enough in my understanding to not add confusion instead of clarity

The following sections "Hooks, part 1", "Hooks, part2" and "Writing Your Own Hooks" name hooks appropriately. The section you were reading was getting across the flow of hooks and not not focusing on what any of the hooks would be doing.