Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Removed unused setSVG and hash functionality from pixmapsource #13423

Merged
merged 3 commits into from
Jul 1, 2024

Conversation

JoergAtGithub
Copy link
Member

Instead use the getPath explicit as in key
Improved cache key string concatenation

Instead use the getPath explicit as in key.
Improved cache key string concatenation
@github-actions github-actions bot added the ui label Jun 30, 2024
Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

Thanks for the (likely) improvement.

@Swiftb0y
Copy link
Member

I haven't read the imagestore code too closely. Does it cache the data persistently across mixxx restarts? If thats the case, it would likely annoy people during development when they adjust and image but its not updated because the old data still lingers in the cache.

@JoergAtGithub
Copy link
Member Author

This behavior is not changed. And this is one of the reasons why I clean up this code step by step. Sometimes Mixxx starts to load SVG again and the UI stucks, see #12904

@Swiftb0y
Copy link
Member

Swiftb0y commented Jul 1, 2024

This behavior is not changed.

Changing the cachekey from being based on the SVG content to being based on the file path does change when the cache is invalidated. Are you sure that the behavior is unchanged?

@JoergAtGithub
Copy link
Member Author

Changing the cachekey from being based on the SVG content to being based on the file path does change when the cache is invalidated. Are you sure that the behavior is unchanged?

There were 2 ways to generate the key:

  1. Using the the file path of the SVG
  2. Hashing the SVG content

The code path that hashed the SVG content was not used (only setSVG could write m_svgSourceData and I there was no caller for this function). therefore I deleted the getKey function in this PR:

QString PixmapSource::getId() const {
if (m_svgSourceData.isEmpty()) {
DEBUG_ASSERT(!m_path.isEmpty());
return m_path;
}
return QCryptographicHash::hash(m_svgSourceData, QCryptographicHash::Sha1).toHex();
}

Instead of getKey I use the getPath function, which is the same as the used code path of getKey:

const QString& PixmapSource::getPath() const {
return m_path;
}

Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

I see. Thank you. LGTM

@Swiftb0y Swiftb0y merged commit ddf04fb into mixxxdj:main Jul 1, 2024
13 checks passed
@JoergAtGithub JoergAtGithub deleted the cleanupPixmapSource branch July 1, 2024 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants