start working on themes #479

bilelmoussaoui · 2017-05-13T14:16:50Z

Should fix #299 #240 once it's ready
This shouldn't be merged yet, more commits are coming this evening!

… PREFIX/ArticleView instead of grsource

jangernert · 2017-05-13T16:52:59Z

src/ArticleTheme.vala

+
+    public FeedReader.ArticleTheme() {
+      // Local themes
+      string theme_dir = GLib.Environment.get_home_dir() + "/.feedreader/themes/";


I'm pretty sure you can't use this path with flatpaks. Also there is already $HOME/.local/feedreader.
You can access that via GLib.Environment.get_user_data_dir() + "/feedreader/"

I will fix that :)

bilelmoussaoui · 2017-05-13T18:27:57Z

@jangernert Everything should work fine, except a small issue that i couldn't figure out (little tired :/)
The installed themes are correctly detected on both locations global and local one, the fallback theme (saved on gresource works fine too), but the length of the static variable themes on ArticleTheme is always empty (i've added some printing on my local machine and the themes are correctly detected.. but not added to the static Variable)
My Vala skills aren't that good as yours, can you take a look?
Thanks!
Edit: already fixed it on my local machine, i will push that to themes branch with other fixes :)

brendanlong

The ability for users to load third-party themes looks pretty cool!

I took a look through the pull request and thought you might be interested in some feedback. Let me know if any of this is unhelpful.

brendanlong · 2017-05-22T21:20:43Z

src/ArticleTheme.vala

+                  }
+                }
+              }catch(Error err){
+                corrupted_theme = true;


It would be good to log the error here for debugging purposes.

brendanlong · 2017-05-22T21:22:19Z

src/ArticleTheme.vala

+        while ((name = theme_dir.read_name()) != null){
+          if (name == "theme.json"){
+              corrupted_theme = false;
+              string path = Path.build_filename(theme_path, name);


It looks like we're reading through the directory but only doing something with one file. Could we replace this while loop with:

// etc. string author = ""; try { string path = Path.build_filename(theme_path, "theme.json"); Json.Parser parser = new Json.Parser(); // etc.

The code here is still not complete, i would like to check if there's an article.html & style.css file too. Otherwise the theme will be correct. But yeah it's better to use this 👍

brendanlong · 2017-05-22T21:22:36Z

src/ArticleTheme.vala

+          }
+        }
+      }catch(GLib.FileError err){
+


Logging something here would be nice too.

brendanlong · 2017-05-22T21:24:11Z

src/ArticleTheme.vala

+using Gee;
+
+public class  FeedReader.ArticleTheme {
+    private static ArrayList<HashMap<string, string>> ? themes = null;


Consider making the ThemeInfo into a struct:

public struct ThemeInfo { string name; string author; bool is_corrupted; // etc. } private static ArrayList<ThemeInfo>? themes = null; // etc.

Since we know the contents of the map, using a struct is more efficient and easier to work with.

You might also consider making this a map from theme name to theme info so we can more easily lookup themes:

private static HashMap<string, ThemeInfo>? = null; public static HashMap<string, ThemeInfo> getThemes() { ... } public static ThemeInfo? getTheme(string name) { var themes = getThemes(); if (!themes.has(name)) return null; return themes.get(name); } public static bool exists(string name) { return getTheme(name) != null; }

brendanlong · 2017-05-22T21:25:23Z

src/ArticleTheme.vala

+            string path = Path.build_filename(location, name);
+            if(FileUtils.test(path, FileTest.IS_DIR)){
+              var themeInfo = getThemeInfo(path);
+              if (themeInfo.has_key("corrupted") == false){


Since this is the only thing we use "corrupted" for, consider just returning null when the theme is corrupt.

Yeah much better!

brendanlong · 2017-05-22T21:25:36Z

src/ArticleTheme.vala

+            }
+          }
+      } catch (GLib.FileError err){
+


One more case where logging would be nice (or just don't catch the exception).

brendanlong · 2017-05-22T21:27:11Z

src/UtilsUI.vala

+    string css = "";
+    try
+    {
+        if (ArticleTheme.isExists(theme) == false) {


It would be simpler to do if (!ArtistTheme.isExists(theme))

Javascript habits :p

brendanlong · 2017-05-22T21:27:29Z

src/ArticleTheme.vala

+      }
+    }
+
+    public static bool isExists(string theme_location){


I would prefer if this was named exists()

brendanlong · 2017-05-22T21:29:33Z

src/UtilsUI.vala

+    string css_id = "$CSS";
+    int css_pos = article.str.index_of(css_id);
+    article.erase(css_pos, css_id.length);
+    article.insert(css_pos, css);


Would string.replace() work here?

It was used before by @jangernert, i will see what can i do. But i was going to remove including the CSS file here as it's not needed. it's even better to just load the html file, this way third-party themes could contain images, fonts that we shouldn't load by ourselves (vala code)

brendanlong · 2017-05-22T21:31:37Z

data/ArticleView/default/article.html

+                    <div id="title" target="_blank" style="font-size:$LARGESIZEpt;"><a href="$URL">$TITLE</a></div>
+                    <div id="author" style="font-size:$SMALLSIZEpt;">
+                         $AUTHOR
+                    </div>


Would it be make sense to use more semantic tags like <h2> instead of <div>?

The theme isn't ready, it's just an example of how things works 👍
Once the Vala code is ready, i will work on two/three themes that will replace the default ones

Oh cool, that makes sense :)

bilelmoussaoui · 2017-05-24T21:56:50Z

@brendanlong Thanks for the review again! I've pushed few modifications on my latest commit :)

brendanlong · 2017-05-25T04:37:50Z

src/ArticleTheme.vala

 public class  FeedReader.ArticleTheme {
-    private static ArrayList<HashMap<string, string>> ? themes = null;
+    private static HashMap<string, ThemeInfo?> ? themes = null;


Why is the second type argument ThemeInfo?? Are there cases where we would want them theme to null?

Not at all, it's just valac complained about not using themeInfo?

bilelmoussaoui · 2017-05-25T21:16:07Z

@jangernert Can you see if everything is alright for you? 👍

jangernert · 2017-10-23T14:27:58Z

Just wanted to mention the cmake files ^^
As default we should imo straight up port the current default theme and just strip out all the color variants

bilelmoussaoui · 2017-10-23T16:30:40Z

@jangernert I will do that :)

jangernert · 2017-10-23T18:38:38Z

Code seems fine and worked flawlessly for me. The only thing missing is to have the 2 themes on the same quality level as the current themes. Otherwise it will be a downgrade from a user point of view.

bilelmoussaoui · 2017-10-23T18:47:04Z

Yeah, lt's what i will work on those days :)

start working on themes

cd551bc

bilelmoussaoui requested a review from jangernert May 13, 2017 14:16

bilelmoussaoui added 4 commits May 13, 2017 17:39

add themes json file, read local and global themes, install themes on…

94fd767

… PREFIX/ArticleView instead of grsource

few fixes

46a2b88

remove unneeded class

c648d28

always load article from file, gresource not used anymore

d9e9e18

jangernert reviewed May 13, 2017

View reviewed changes

bilelmoussaoui added 2 commits May 13, 2017 19:23

fix flatpak folder, and add a fallback theme, fix building

de0d630

more fixes

38f7a11

bilelmoussaoui added this to the 2.2 milestone May 13, 2017

bilelmoussaoui added 2 commits May 13, 2017 21:10

few fixes, works perfectly

8b633eb

Merge branch 'master' into themes

f33303e

brendanlong reviewed May 22, 2017

View reviewed changes

use struct for themeInfo

d8e046d

brendanlong reviewed May 25, 2017

View reviewed changes

bilelmoussaoui mentioned this pull request May 25, 2017

Use git submodule for grabberconfig #493

Merged

jangernert force-pushed the master branch 2 times, most recently from 1171648 to c147082 Compare September 1, 2017 00:42

bilelmoussaoui added 3 commits October 23, 2017 16:19

update to master

6fdb4d9

remove UtilsUI and fix merge confilicts

a849460

remove old source files from CMakeLists

d976d49

bilelmoussaoui added 4 commits October 23, 2017 16:47

restore the GrabberConfig

8f5aae5

update Utils & catch GLib.Error on ArticleTheme

086e6b6

space2tabs

4100269

fix themes loading, add pre/code tags theming, fix img width & height

6bb6fac

brendanlong force-pushed the master branch 2 times, most recently from 50b6d96 to 879f9c3 Compare May 1, 2018 16:04

bilelmoussaoui mentioned this pull request Oct 8, 2018

Add new article themes. #765

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

start working on themes #479

start working on themes #479

bilelmoussaoui commented May 13, 2017 •

edited

jangernert May 13, 2017

bilelmoussaoui May 13, 2017

bilelmoussaoui commented May 13, 2017 •

edited

brendanlong left a comment

brendanlong May 22, 2017

brendanlong May 22, 2017

bilelmoussaoui May 23, 2017

brendanlong May 22, 2017

brendanlong May 22, 2017

brendanlong May 22, 2017

bilelmoussaoui May 23, 2017

brendanlong May 22, 2017

brendanlong May 22, 2017

bilelmoussaoui May 23, 2017

brendanlong May 22, 2017

brendanlong May 22, 2017

bilelmoussaoui May 23, 2017

brendanlong May 23, 2017

brendanlong May 22, 2017

bilelmoussaoui May 23, 2017

brendanlong May 23, 2017

bilelmoussaoui commented May 24, 2017

brendanlong May 25, 2017

bilelmoussaoui May 25, 2017

bilelmoussaoui commented May 25, 2017

jangernert commented Oct 23, 2017

bilelmoussaoui commented Oct 23, 2017

jangernert commented Oct 23, 2017

bilelmoussaoui commented Oct 23, 2017

start working on themes #479

Are you sure you want to change the base?

start working on themes #479

Conversation

bilelmoussaoui commented May 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bilelmoussaoui commented May 13, 2017 • edited

brendanlong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bilelmoussaoui commented May 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bilelmoussaoui commented May 25, 2017

jangernert commented Oct 23, 2017

bilelmoussaoui commented Oct 23, 2017

jangernert commented Oct 23, 2017

bilelmoussaoui commented Oct 23, 2017

bilelmoussaoui commented May 13, 2017 •

edited

bilelmoussaoui commented May 13, 2017 •

edited