-
Notifications
You must be signed in to change notification settings - Fork 42
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
Rework UI in preparation for multiple sources #143
Conversation
bb05e2a
to
b3bc5f8
Compare
@@ -1,8 +1,10 @@ | |||
#!/bin/bash | |||
|
|||
datahome="${XDG_DATA_HOME:-HOME/.local/share}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, that's embarrassing - I missed a $
before HOME, just recognized this while looking in the description of #144 where I wrote the example correctly…
Another fixing commit for the last PR, sry!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That already looks like a lot of great work! Thanks! 😃
The UI is a bit rough here and there and needs improvements in the end, but it already looks like a great start towards a new version!
I just have some smaller comments & questions.
Just a high-level note: there are some changes unrelated to the commits. For example, formatting changes or .gitignore changes that would be better to have in a separate commit. I am fine with this for now since I haven't been super exact about this myself in this repository in the past. However, I might be pickier about that in the future (probably only after all your current PRs are merged 😄).
[email protected]/schemas/org.gnome.shell.extensions.space.iflow.randomwallpaper.gschema.xml
Show resolved
Hide resolved
let identifier = this._settings.get("generic-json-id", "string"); | ||
let imageJSONPath = this._settings.get("generic-json-image-path", "string"); | ||
let postJSONPath = this._settings.get("generic-json-post-path", "string"); | ||
let domainUrl = this._settings.get("generic-json-domain", "string"); | ||
let authorNameJSONPath = this._settings.get("generic-json-author-name-path", "string"); | ||
let authorUrlJSONPath = this._settings.get("generic-json-author-url-path", "string"); | ||
|
||
if (identifier === null || identifier === "") { | ||
identifier = 'Generic JSON Source'; | ||
} | ||
|
||
let rObject = this._jsonPathParser.access(response_body, imageJSONPath); | ||
let imageDownloadUrl = this._settings.get("generic-json-image-prefix", "string") + rObject.Object; | ||
|
||
// '@random' would yield different results so lets make sure the values stay | ||
// the same as long as the path is identical | ||
let samePath = imageJSONPath.substring(0, this.findFirstDifference(imageJSONPath, postJSONPath)); | ||
|
||
// count occurrences of '@random' to slice the array later | ||
// https://stackoverflow.com/a/4009768 | ||
let occurrences = (samePath.match(/@random/g) || []).length; | ||
let slicedRandomElements = rObject.RandomElements.slice(0, occurrences); | ||
|
||
let postUrl = this._jsonPathParser.access(response_body, postJSONPath, slicedRandomElements, false).Object; | ||
postUrl = this._settings.get("generic-json-post-prefix", "string") + postUrl; | ||
if (typeof postUrl !== 'string' || !postUrl instanceof String) { | ||
postUrl = null; | ||
} | ||
|
||
let authorName = this._jsonPathParser.access(response_body, authorNameJSONPath, slicedRandomElements, false).Object; | ||
if (typeof authorName !== 'string' || !authorName instanceof String) { | ||
authorName = null; | ||
} | ||
|
||
let authorUrl = this._jsonPathParser.access(response_body, authorUrlJSONPath, slicedRandomElements, false).Object; | ||
authorUrl = this._settings.get("generic-json-author-url-prefix", "string") + authorUrl; | ||
if (typeof authorUrl !== 'string' || !authorUrl instanceof String) { | ||
authorUrl = null; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you wanted to achieve here! However, I think we need a fundamentally better approach to handling the meta data (author, post link, etc.) of the images.
Also, I think we can make the JsonPath part little bit easier (see the other comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear, it is fine for now to add the multiple JSON path fields for the meta data, but we might find a better way in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me, the generic JSON source didn't work and threw an error BaseAdapter :: {"error":"Unexpected response. (TypeError: rObject.RandomElements is null)"}
I didn't debug the issue. Should that already work, or are there fixes for this in later PRs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should work! I'll have a look at it, maybe something got lost while transfering the commits between the branches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, it works just fine here.
In the past I had the problem that the gnome extension app was overwriting my local folder on gnome shell restart - maybe that's interfering on your side?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, it worked after restting my stored settings with dconf... That's somewhat unfortunate since I can't restore my previous state to test what caused the issue. We could change the schema id in the end so that other users do not run into the same issue with the next version.
Fyi, created an issue for tracking here: #156
Introduces new function to resolve a new random path according to an already resolved path with similar elements. Reference: ifl0w#143 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so this one is done! Great work, thanks! One down, 12 to go. 😅
I just changed the base branch, so we do not break the extension for any users that have cloned the develop branch directly. We'll collect all changes in the WIP_3.0.0 branch and merge them in develop once we have a somewhat stable version.
Introduces new function to resolve a new random path according to an already resolved path with similar elements. Reference: ifl0w#143 (comment)
* Prefix HOME variable to correctly get the users home path * Prevent whitespace issues by quoting the users home path variable Reference: ifl0w#143 (comment)
Introduces new function to resolve a new random path according to an already resolved path with similar elements. Reference: ifl0w#143 (comment)
* Prefix HOME variable to correctly get the users home path * Prevent whitespace issues by quoting the users home path variable Reference: ifl0w#143 (comment)
Introduces new function to resolve a new random path according to an already resolved path with similar elements. Reference: #143 (comment)
* Prefix HOME variable to correctly get the users home path * Prevent whitespace issues by quoting the users home path variable Reference: #143 (comment)
Introduces new function to resolve a new random path according to an already resolved path with similar elements. Reference: #143 (comment)
* Prefix HOME variable to correctly get the users home path * Prevent whitespace issues by quoting the users home path variable Reference: #143 (comment)
Introduces new function to resolve a new random path according to an already resolved path with similar elements. Reference: #143 (comment)
* Prefix HOME variable to correctly get the users home path * Prevent whitespace issues by quoting the users home path variable Reference: #143 (comment)
Conflicts:
[email protected]/metadata.json
[email protected]/prefs.js
This should be a drop-in replacement and is a preparation for #141
If you have any comments please don't hesitate to mention them but keep in mind, that it might be already changed in the next PR. (Example is the enum change to int, which I changed back later back to an enum)
I'll make sure then that the requested changes will appear in the last stacked PR (change to TS).
Original message:
This rebuilds the UI in the blueprint-compiler language. I've seen you mentioning problems with the UI-files before while using glade, so this might be easier to maintain.
Building the UI requires
blueprint-compiler
.This includes some enhancements to the GenericJSON. It now exposes more of the history element options and tries to match the same
$[@random]
across all paths until they deviate.This probably isn't tested enough.