Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(hash): surface source URLs on the Supported Game Files page #3011

Closed
wants to merge 2 commits into from
+106 −17
Closed

feat(hash): surface source URLs on the Supported Game Files page #3011

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e58ad50
feat(hash): surface source URLs on the Supported Game Files page
wescopeland Jan 1, 2025
7731679
chore: tsc
wescopeland Jan 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions app/Platform/Data/GameHashData.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ public function __construct(
#[TypeScriptType('App\\Platform\\Data\\GameHashLabelData[]')]
public array $labels,
public ?string $patchUrl,
/** "Resource Page URL" */
public ?string $source,
) {
}

Expand All @@ -31,6 +33,7 @@ public static function fromGameHash(GameHash $gameHash): self
name: $gameHash->name,
labels: GameHashLabelData::fromLabelsString($gameHash->labels),
patchUrl: $gameHash->patch_url,
source: $gameHash->source,
);
}

Expand Down
4 changes: 3 additions & 1 deletion lang/en_US.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -520,5 +520,7 @@
"This page may contain content that is not appropriate for all ages.": "This page may contain content that is not appropriate for all ages.",
"Are you sure you want to view this page?": "Are you sure you want to view this page?",
"Yes, and don't ask again": "Yes, and don't ask again",
"Yes, I'm an adult": "Yes, I'm an adult"
"Yes, I'm an adult": "Yes, I'm an adult",
"Download from Original Source (Recommended)": "Download from Original Source (Recommended)",
"Mirror": "Mirror"
}
89 changes: 75 additions & 14 deletions resources/js/features/games/components/HashesMainRoot/HashesList/HashesList.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,20 +68,6 @@ describe('Component: HashesList', () => {
expect(screen.getByText(hash.md5)).toBeVisible();
});

it('given the hash has a patch URL, adds a link to it', () => {
// ARRANGE
const hash = createGameHash({ patchUrl: faker.internet.url() });

render<App.Platform.Data.GameHashesPageProps>(<HashesList />, {
pageProps: { hashes: [hash] },
});

// ASSERT
const linkEl = screen.getByRole('link', { name: /download patch file/i });
expect(linkEl).toBeVisible();
expect(linkEl).toHaveAttribute('href', hash.patchUrl);
});

it('given the hash has no patch URL, does not display a download link', () => {
// ARRANGE
const hash = createGameHash({ patchUrl: null });
Expand Down Expand Up @@ -186,4 +172,79 @@ describe('Component: HashesList', () => {
expect(renderedMd5s[1]).toContain('77057d9d14b99e465ea9e29783af0ae3');
expect(renderedMd5s[2]).toContain('a78d58b97eddb7c70647d939e20bef4f');
});

it('given the hash has both source and patch URLs, displays links to both', () => {
// ARRANGE
const hash = createGameHash({
source: faker.internet.url(),
patchUrl: faker.internet.url(),
});

render<App.Platform.Data.GameHashesPageProps>(<HashesList />, {
pageProps: { hashes: [hash] },
});

// ASSERT
const links = screen.getAllByRole('link');
expect(links[0]).toHaveAttribute('href', hash.source);
expect(links[1]).toHaveAttribute('href', hash.patchUrl);

expect(screen.getByText(/download from original source/i)).toBeVisible();
expect(screen.getByText(/mirror/i)).toBeVisible();
});

it('given the hash only has a patch URL, displays a single download link', () => {
// ARRANGE
const hash = createGameHash({
source: null,
patchUrl: faker.internet.url(),
});

render<App.Platform.Data.GameHashesPageProps>(<HashesList />, {
pageProps: { hashes: [hash] },
});

// ASSERT
const link = screen.getByRole('link');
expect(link).toHaveAttribute('href', hash.patchUrl);
expect(screen.getByText(/download patch file/i)).toBeVisible();

expect(screen.queryByText(/mirror/i)).not.toBeInTheDocument();
expect(screen.queryByText(/download from original source/i)).not.toBeInTheDocument();
});

it('given the hash only has a source URL, does not display any links', () => {
// ARRANGE
const hash = createGameHash({
source: faker.internet.url(),
patchUrl: null,
});

render<App.Platform.Data.GameHashesPageProps>(<HashesList />, {
pageProps: { hashes: [hash] },
});

// ASSERT
expect(screen.queryByRole('link')).not.toBeInTheDocument();
});

it('given the hash only has a patch URL, displays a single download link', () => {
// ARRANGE
const hash = createGameHash({
source: null,
patchUrl: faker.internet.url(),
});

render<App.Platform.Data.GameHashesPageProps>(<HashesList />, {
pageProps: { hashes: [hash] },
});

// ASSERT
const link = screen.getByRole('link');
expect(link).toHaveAttribute('href', hash.patchUrl);
expect(screen.getByText(/download patch file/i)).toBeVisible();

expect(screen.queryByText(/mirror/i)).not.toBeInTheDocument();
expect(screen.queryByText(/download from original source/i)).not.toBeInTheDocument();
});
});
23 changes: 22 additions & 1 deletion resources/js/features/games/components/HashesMainRoot/HashesList/HashesListItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import type { FC } from 'react';
import { useTranslation } from 'react-i18next';

import { buildTrackingClassNames } from '@/common/utils/buildTrackingClassNames';
import { cn } from '@/common/utils/cn';

interface HashListingProps {
hash: App.Platform.Data.GameHash;
Expand All @@ -27,7 +28,27 @@ export const HashesListItem: FC<HashListingProps> = ({ hash }) => {
<div className="flex flex-col border-l-2 border-neutral-700 pl-2 light:border-embed-highlight black:border-neutral-700">
<p className="font-mono text-neutral-200 light:text-neutral-700">{hash.md5}</p>

{hash.patchUrl ? (
{/* Can show RAPatches as the mirror */}
{hash.source && hash.patchUrl ? (
<div className="mt-1 flex flex-col">
<a
href={hash.source}
className={cn(buildTrackingClassNames('Open Patch Source URL', { md5: hash.md5 }))}
>
{t('Download from Original Source (Recommended)')}
</a>
Copy link
Member

Choose a reason for hiding this comment

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

What if the original source gets a newer version?

I thought we had our own repository in case the original went away or got updated.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a fair point. I don't know if we can have a good solution if and when that happens. @televandalist do you have any thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just chiming in with my two cents from the perspective of a hack author. Better visibility of the hack source, if anything, can lead players to more easily identify and ask for a newer version to be linked on the RA side or for a legacy RA version to be rehosted at the hack source at least until the former happens.

If the hack author is active, the hack author should be able to rehost a legacy RA version if the hack author is willing. If the hack author is no longer active, this means that the hack versioning has stabilized, and RA is more likely to be able to catch up in linked version or to have the stabilized version already linked.

I realize that devs are under no obligation to catch the set up in any timeframe (the same applies to hack authors rehosting legacy versions), but players on RA do tend to want to be able to play the latest version.

How about we label the hack source as "(Recommended; check the hack's version in case of updates)" and add a tooltip that says, "If there is a version mismatch, use the mirror link in the meantime and consider contacting the hack author or replying to the RA forum topic to get the versions to match."?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not a fan of adding this to every hack link that has a resource URL.

Ideally, hack authors and RA developers collaborate on this sort of thing. I don't want RA users taking it upon themselves to reach out to hack authors to remediate version mismatches on RA. We have seen how RA users reaching out to emulator developers has historically gone.

I would like @televandalist to weigh in on this comment thread before I take any further action.

Copy link
Member

Choose a reason for hiding this comment

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

Better visibility of the hack source, if anything, can lead players to more easily identify and ask for a newer version to be linked on the RA side or for a legacy RA version to be rehosted at the hack source at least until the former happens.

I really don't think that this is something that can be universally applied. It ultimately falls on the set developer(s) to figure out and handle as they see fit. Some newer versions of hacks, translations, and even homebrews are intentionally ignored and some aren't linked simply because a set lacks an active dev/maintainer. While it's still new, we should encourage compatibility testing over flooding the forums, walls, and/or DMs with requests to update hashes. Most players aren't aware of what all goes into set development; which compatibility testing makes irrelevant.

If the hack author is active, the hack author should be able to rehost a legacy RA version if the hack author is willing. If the hack author is no longer active, this means that the hack versioning has stabilized, and RA is more likely to be able to catch up in linked version or to have the stabilized version already linked.

I believe this would further complicate the entire process and adds way too much reliance on certain people not having a bad day. I have more than enough on my plate so adding "check if hack author is still active and if versioning has stabilized" to each RAPatches upload is not something I'd be willing to do. My interest in all of this starts and ends with whether hashes are accounted for, verified, and can be reproduced by someone without them pulling their hair out. I also believe that if having a patch in the repo is problematic to the author, then maybe the hash shouldn't even be linked.

I realize that devs are under no obligation to catch the set up in any timeframe (the same applies to hack authors rehosting legacy versions), but players on RA do tend to want to be able to play the latest version.

Again, it's not something that's universal and players can check out compatibility testing.

How about we label the hack source as "(Recommended; check the hack's version in case of updates)" and add a tooltip that says, "If there is a version mismatch, use the mirror link in the meantime and consider contacting the hack author or replying to the RA forum topic to get the versions to match."?

I'm not a fan of adding this to every hack link that has a resource URL.

I think it would be better to just add more info to this section:
Capture

Maybe something like this:

"This page shows which ROM hashes are compatible with this entry. Additional information regarding these hashes may be listed in the entry's official forum topic. Details on how hashes are generated for each system can be found here.

Please contact active set developer(s) regarding any errors. If there is not an active developer associated with the entry, then contact QATeam.

Information on contributing with compatibility testing can be found here."

Copy link
Member

Choose a reason for hiding this comment

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

What if the original source gets a newer version?

I thought we had our own repository in case the original went away or got updated.

That's a fair point. I don't know if we can have a good solution if and when that happens. @televandalist do you have any thoughts?

Not really since the info is tied to individual hashes. Adding a note that patch sources may not be entirely up-to-date seems like it would defeat the purpose 😆


<a
href={hash.patchUrl}
className={cn(buildTrackingClassNames('Download Patch File', { md5: hash.md5 }))}
>
{t('Mirror')}
</a>
</div>
) : null}

{/* Show RAPatches as the direct download link */}
{!hash.source && hash.patchUrl ? (
<a
href={hash.patchUrl}
className={buildTrackingClassNames('Download Patch File', { md5: hash.md5 })}
Expand Down
1 change: 1 addition & 0 deletions resources/js/test/factories/createGameHash.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,5 +15,6 @@ export const createGameHash = createFactory<App.Platform.Data.GameHash>((faker)
md5: faker.string.alphanumeric(32),
name: faker.word.words(3),
patchUrl: faker.internet.url(),
source: null,
};
});
3 changes: 2 additions & 1 deletion resources/js/types/generated.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ declare namespace App.Data {
};
}
declare namespace App.Enums {
export type ClientSupportLevel = 0 | 1 | 2 | 3;
export type ClientSupportLevel = 0 | 1 | 2 | 3 | 4;
export type UserPreference =
| 0
| 1
Expand Down Expand Up @@ -353,6 +353,7 @@ declare namespace App.Platform.Data {
name: string | null;
labels: Array<App.Platform.Data.GameHashLabel>;
patchUrl: string | null;
source: string | null;
};
export type GameHashLabel = {
label: string;
Expand Down