Improve GuildChannel#getPosition and Guild#getChannels (#2320)

src/main/java/net/dv8tion/jda/api/entities/channel/attribute/IPositionableChannel.java

@@ -20,8 +20,6 @@
 import net.dv8tion.jda.api.managers.channel.attribute.IPositionableChannelManager;
 
 import javax.annotation.Nonnull;
-import java.util.List;
-import java.util.stream.Collectors;
 
 /**
  * Represents a {@link GuildChannel GuildChannel} that has a position.
@@ -37,13 +35,14 @@ public interface IPositionableChannel extends GuildChannel
     @Nonnull
     IPositionableChannelManager<?, ?> getManager();
 
-    //TODO-v5: We should probably reconsider how getPosition is calculated as it isn't particularly useful anymore...
-    //TODO-v5: We should probably introduce getPositionAbsolute that returns the index of the channel in Guild#getChannels
     //TODO-v5: We should probably introduce getPositionInCategory (name pending) that returns index in Category#getChannels or -1
+
     /**
-     * The position this GuildChannel is displayed at.
-     * <br>Higher values mean they are displayed lower in the Client. Position 0 is the top most GuildChannel
-     * Channels of a {@link net.dv8tion.jda.api.entities.Guild Guild} do not have to have continuous positions
+     * The position of this channel in the channel list of the guild.
+     * <br>This does not account for thread channels, as they do not have positions.
+     *
+     * <p>This is functionally equivalent to {@code getGuild().getChannels().indexOf(channel)}.
+     * To efficiently compare the position between channels, it is recommended to use {@link #compareTo(Object)} instead of the position.
      *
      * @throws IllegalStateException
      *         If this channel is not in the guild cache
@@ -52,23 +51,16 @@ public interface IPositionableChannel extends GuildChannel
      */
     default int getPosition()
     {
-        int sortBucket = getType().getSortBucket();
-        List<GuildChannel> channels = getGuild().getChannels().stream()
-            .filter(chan -> chan.getType().getSortBucket() == sortBucket)
-            .sorted()
-            .collect(Collectors.toList());
-
-        for (int i = 0; i < channels.size(); i++)
-        {
-            if (equals(channels.get(i)))
-                return i;
-        }
+        int position = getGuild().getChannels().indexOf(this);
+        if (position > -1)
+            return position;
         throw new IllegalStateException("Somehow when determining position we never found the " + getType().name() + " in the Guild's channels? wtf?");
     }
 
     /**
      * The actual position of the {@link GuildChannel GuildChannel} as stored and given by Discord.
-     * Channel positions are actually based on a pairing of the creation time (as stored in the snowflake id)
+     *
+     * <p>Channel positions are actually based on a pairing of the creation time (as stored in the snowflake id)
      * and the position. If 2 or more channels share the same position then they are sorted based on their creation date.
      * The more recent a channel was created, the lower it is in the hierarchy. This is handled by {@link #getPosition()}
      * and it is most likely the method you want. If, for some reason, you want the actual position of the

src/main/java/net/dv8tion/jda/internal/entities/GuildImpl.java

@@ -747,72 +747,33 @@ public SnowflakeCacheView<GuildSticker> getStickerCache()
     public List<GuildChannel> getChannels(boolean includeHidden)
     {
         Member self = getSelfMember();
-        Predicate<GuildChannel> filterHidden = it -> self.hasPermission(it, Permission.VIEW_CHANNEL);
-
-        List<GuildChannel> channels;
-        SnowflakeCacheViewImpl<Category> categoryView = getCategoriesView();
-        SnowflakeCacheViewImpl<VoiceChannel> voiceView = getVoiceChannelsView();
-        SnowflakeCacheViewImpl<StageChannel> stageView = getStageChannelsView();
-        SnowflakeCacheViewImpl<TextChannel> textView = getTextChannelsView();
-        SnowflakeCacheViewImpl<NewsChannel> newsView = getNewsChannelView();
-        SnowflakeCacheViewImpl<ForumChannel> forumView = getForumChannelsView();
-        List<TextChannel> textChannels;
-        List<NewsChannel> newsChannels;
-        List<VoiceChannel> voiceChannels;
-        List<StageChannel> stageChannels;
-        List<ForumChannel> forumChannels;
-        List<Category> categories;
-        try (UnlockHook categoryHook = categoryView.readLock();
-             UnlockHook voiceHook = voiceView.readLock();
-             UnlockHook textHook = textView.readLock();
-             UnlockHook newsHook = newsView.readLock();
-             UnlockHook stageHook = stageView.readLock();
-             UnlockHook forumHook = forumView.readLock())
-        {
-            if (includeHidden)
-            {
-                textChannels = textView.asList();
-                newsChannels = newsView.asList();
-                voiceChannels = voiceView.asList();
-                stageChannels = stageView.asList();
-                forumChannels = forumView.asList();
-            }
-            else
-            {
-                textChannels = textView.stream().filter(filterHidden).collect(Collectors.toList());
-                newsChannels = newsView.stream().filter(filterHidden).collect(Collectors.toList());
-                voiceChannels = voiceView.stream().filter(filterHidden).collect(Collectors.toList());
-                stageChannels = stageView.stream().filter(filterHidden).collect(Collectors.toList());
-                forumChannels = forumView.stream().filter(filterHidden).collect(Collectors.toList());
-            }
-            categories = categoryView.asList(); // we filter categories out when they are empty (no visible channels inside)
-            channels = new ArrayList<>((int) categoryView.size() + voiceChannels.size() + textChannels.size() + newsChannels.size() + stageChannels.size());
-        }
+        Predicate<GuildChannel> filterHidden = includeHidden ? (it) -> true : it -> self.hasPermission(it, Permission.VIEW_CHANNEL);
 
-        textChannels.stream().filter(it -> it.getParentCategory() == null).forEach(channels::add);
-        newsChannels.stream().filter(it -> it.getParentCategory() == null).forEach(channels::add);
-        voiceChannels.stream().filter(it -> it.getParentCategory() == null).forEach(channels::add);
-        stageChannels.stream().filter(it -> it.getParentCategory() == null).forEach(channels::add);
-        forumChannels.stream().filter(it -> it.getParentCategory() == null).forEach(channels::add);
-        Collections.sort(channels);
+        SnowflakeCacheViewImpl<Category> categories = getCategoriesView();
+        SnowflakeCacheViewImpl<VoiceChannel> voice = getVoiceChannelsView();
+        SnowflakeCacheViewImpl<StageChannel> stage = getStageChannelsView();
+        SnowflakeCacheViewImpl<TextChannel> text = getTextChannelsView();
+        SnowflakeCacheViewImpl<NewsChannel> news = getNewsChannelView();
+        SnowflakeCacheViewImpl<ForumChannel> forum = getForumChannelsView();
+
+        List<GuildChannel> channels = new ArrayList<>((int) (categories.size() + voice.size() + stage.size() + text.size() + news.size() + forum.size()));
+
+        voice.acceptStream(stream -> stream.filter(filterHidden).forEach(channels::add));
+        stage.acceptStream(stream -> stream.filter(filterHidden).forEach(channels::add));
+        text.acceptStream(stream -> stream.filter(filterHidden).forEach(channels::add));
+        news.acceptStream(stream -> stream.filter(filterHidden).forEach(channels::add));
+        forum.acceptStream(stream -> stream.filter(filterHidden).forEach(channels::add));
 
-        for (Category category : categories)
+        categories.forEach(category ->
         {
-            List<GuildChannel> children;
-            if (includeHidden)
-            {
-                children = category.getChannels();
-            }
-            else
-            {
-                children = category.getChannels().stream().filter(filterHidden).collect(Collectors.toList());
-                if (children.isEmpty())
-                    continue;
-            }
+            if (!includeHidden && category.getChannels().stream().noneMatch(filterHidden))
+                return;
 
             channels.add(category);
-            channels.addAll(children);
-        }
+        });
+
+        // See AbstractGuildChannelImpl#compareTo for details on how this achieves the canonical order of the client
+        Collections.sort(channels);
 
         return Collections.unmodifiableList(channels);
     }

src/main/java/net/dv8tion/jda/internal/entities/channel/middleman/AbstractGuildChannelImpl.java

@@ -16,7 +16,10 @@
 
 package net.dv8tion.jda.internal.entities.channel.middleman;
 
+import net.dv8tion.jda.api.entities.channel.attribute.ICategorizableChannel;
 import net.dv8tion.jda.api.entities.channel.attribute.IPositionableChannel;
+import net.dv8tion.jda.api.entities.channel.concrete.Category;
+import net.dv8tion.jda.api.entities.channel.concrete.ThreadChannel;
 import net.dv8tion.jda.api.entities.channel.middleman.GuildChannel;
 import net.dv8tion.jda.internal.entities.GuildImpl;
 import net.dv8tion.jda.internal.entities.channel.AbstractChannelImpl;
@@ -47,18 +50,67 @@ public int compareTo(@Nonnull GuildChannel o)
     {
         Checks.notNull(o, "Channel");
 
-        // if bucket matters
+        // Check thread positions
+        ThreadChannel thisThread = this instanceof ThreadChannel ? (ThreadChannel) this : null;
+        ThreadChannel otherThread = o instanceof ThreadChannel ? (ThreadChannel) o : null;
+
+        if (thisThread != null && otherThread == null)
+            return thisThread.getParentChannel().compareTo(o);
+        if (thisThread == null && otherThread != null)
+            return this.compareTo(otherThread.getParentChannel());
+        if (thisThread != null)
+        {
+            // If they are threads on the same channel
+            if (thisThread.getParentChannel().equals(otherThread.getParentChannel()))
+                return Long.compare(o.getIdLong(), id); // threads are ordered ascending by age
+            // If they are threads on different channels
+            return thisThread.getParentChannel().compareTo(otherThread.getParentChannel());
+        }
+
+        // Check category positions
+        Category thisParent = this instanceof ICategorizableChannel ? ((ICategorizableChannel) this).getParentCategory() : null;
+        Category otherParent = o instanceof ICategorizableChannel ? ((ICategorizableChannel) o).getParentCategory() : null;
+
+        if (thisParent != null && otherParent == null)
+        {
+            if (o instanceof Category)
+            {
+                // The other channel is the parent category of this channel
+                if (o.equals(thisParent))
+                    return 1;
+                // The other channel is another category
+                return thisParent.compareTo(o);
+            }
+            return 1;
+        }
+        if (thisParent == null && otherParent != null)
+        {
+            if (this instanceof Category)
+            {
+                // This channel is parent of other channel
+                if (this.equals(otherParent))
+                    return -1;
+                // This channel is a category higher than the other channel's parent category
+                return this.compareTo(otherParent); //safe use of recursion since no circular parents exist
+            }
+            return -1;
+        }
+        // Both channels are in different categories, compare the categories instead
+        if (thisParent != null && !thisParent.equals(otherParent))
+            return thisParent.compareTo(otherParent);
+
+        // Check sort bucket (text/message is above audio)
         if (getType().getSortBucket() != o.getType().getSortBucket())
             return Integer.compare(getType().getSortBucket(), o.getType().getSortBucket());
 
-        // if position matters
-        if (o instanceof IPositionableChannel && this instanceof IPositionableChannel) {
+        // Check actual position
+        if (o instanceof IPositionableChannel && this instanceof IPositionableChannel)
+        {
             IPositionableChannel oPositionableChannel = (IPositionableChannel) o;
             IPositionableChannel thisPositionableChannel = (IPositionableChannel) this;
 
-            if (thisPositionableChannel.getPositionRaw() != oPositionableChannel.getPositionRaw()) {
+            if (thisPositionableChannel.getPositionRaw() != oPositionableChannel.getPositionRaw())
                 return Integer.compare(thisPositionableChannel.getPositionRaw(), oPositionableChannel.getPositionRaw());
-            }
         }
 
         // last resort by id